[Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Jan Kiszka]

See patches for details.

Jan Kiszka (2):
  i2c: Introduce device address mask
  Add AT24Cxx I2C EEPROM device model

 hw/Makefile.objs |    2 +-
 hw/at24.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/ds1338.c      |    2 +-
 hw/i2c.c         |    9 +-
 hw/i2c.h         |    3 +-
 hw/lm832x.c      |    2 +-
 hw/max7310.c     |    2 +-
 hw/pxa2xx.c      |    3 +-
 hw/smbus.c       |    2 +-
 hw/ssd0303.c     |    2 +-
 hw/tmp105.c      |    2 +-
 hw/tosa.c        |    2 +-
 hw/twl92230.c    |    2 +-
 hw/wm8750.c      |    2 +-
 hw/z2.c          |    2 +-
 15 files changed, 383 insertions(+), 17 deletions(-)
 create mode 100644 hw/at24.c

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/2] i2c: Introduce device address mask

[Jan Kiszka]

Some devices react on multiple addresses. To emulate this, we could
register them multiple times, but that is cumbersome. The AT24C16, e.g.
listens on 8 different addresses.

Instead, introduce a device address mask that is applied on the
transmitted address before matching it against the stored one. Moreover,
the transmitted address is passed as additional parameter to the event
callback of the device.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/ds1338.c   |    2 +-
 hw/i2c.c      |    9 +++++----
 hw/i2c.h      |    3 ++-
 hw/lm832x.c   |    2 +-
 hw/max7310.c  |    2 +-
 hw/pxa2xx.c   |    3 ++-
 hw/smbus.c    |    2 +-
 hw/ssd0303.c  |    2 +-
 hw/tmp105.c   |    2 +-
 hw/tosa.c     |    2 +-
 hw/twl92230.c |    2 +-
 hw/wm8750.c   |    2 +-
 hw/z2.c       |    2 +-
 13 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/hw/ds1338.c b/hw/ds1338.c
index b576d56..59fcc01 100644
--- a/hw/ds1338.c
+++ b/hw/ds1338.c
@@ -75,7 +75,7 @@ static void inc_regptr(DS1338State *s)
     }
 }
 
-static void ds1338_event(I2CSlave *i2c, enum i2c_event event)
+static void ds1338_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     DS1338State *s = FROM_I2C_SLAVE(DS1338State, i2c);
 
diff --git a/hw/i2c.c b/hw/i2c.c
index 296bece..72b8f07 100644
--- a/hw/i2c.c
+++ b/hw/i2c.c
@@ -93,7 +93,7 @@ int i2c_start_transfer(i2c_bus *bus, uint8_t address, int recv)
     QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
         DeviceState *qdev = kid->child;
         I2CSlave *candidate = I2C_SLAVE_FROM_QDEV(qdev);
-        if (candidate->address == address) {
+        if (candidate->address == (address & candidate->address_mask)) {
             slave = candidate;
             break;
         }
@@ -108,7 +108,7 @@ int i2c_start_transfer(i2c_bus *bus, uint8_t address, int recv)
        start condition.  */
     bus->current_dev = slave;
     if (sc->event) {
-        sc->event(slave, recv ? I2C_START_RECV : I2C_START_SEND);
+        sc->event(slave, recv ? I2C_START_RECV : I2C_START_SEND, address);
     }
     return 0;
 }
@@ -124,7 +124,7 @@ void i2c_end_transfer(i2c_bus *bus)
 
     sc = I2C_SLAVE_GET_CLASS(dev);
     if (sc->event) {
-        sc->event(dev, I2C_FINISH);
+        sc->event(dev, I2C_FINISH, 0);
     }
 
     bus->current_dev = NULL;
@@ -175,7 +175,7 @@ void i2c_nack(i2c_bus *bus)
 
     sc = I2C_SLAVE_GET_CLASS(dev);
     if (sc->event) {
-        sc->event(dev, I2C_NACK);
+        sc->event(dev, I2C_NACK, 0);
     }
 }
 
@@ -207,6 +207,7 @@ static int i2c_slave_qdev_init(DeviceState *dev)
     I2CSlave *s = I2C_SLAVE_FROM_QDEV(dev);
     I2CSlaveClass *sc = I2C_SLAVE_GET_CLASS(s);
 
+    s->address_mask = 0x7f;
     return sc->init(s);
 }
 
diff --git a/hw/i2c.h b/hw/i2c.h
index 0f5682b..6cf164b 100644
--- a/hw/i2c.h
+++ b/hw/i2c.h
@@ -39,7 +39,7 @@ typedef struct I2CSlaveClass
     int (*recv)(I2CSlave *s);
 
     /* Notify the slave of a bus state change.  */
-    void (*event)(I2CSlave *s, enum i2c_event event);
+    void (*event)(I2CSlave *s, enum i2c_event event, uint8_t param);
 } I2CSlaveClass;
 
 struct I2CSlave
@@ -48,6 +48,7 @@ struct I2CSlave
 
     /* Remaining fields for internal use by the I2C code.  */
     uint8_t address;
+    uint8_t address_mask;
 };
 
 i2c_bus *i2c_init_bus(DeviceState *parent, const char *name);
diff --git a/hw/lm832x.c b/hw/lm832x.c
index 8e09f9b..d69aa4b 100644
--- a/hw/lm832x.c
+++ b/hw/lm832x.c
@@ -378,7 +378,7 @@ static void lm_kbd_write(LM823KbdState *s, int reg, int byte, uint8_t value)
     }
 }
 
-static void lm_i2c_event(I2CSlave *i2c, enum i2c_event event)
+static void lm_i2c_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     LM823KbdState *s = FROM_I2C_SLAVE(LM823KbdState, i2c);
 
diff --git a/hw/max7310.c b/hw/max7310.c
index 1ed18ba..63999be 100644
--- a/hw/max7310.c
+++ b/hw/max7310.c
@@ -123,7 +123,7 @@ static int max7310_tx(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void max7310_event(I2CSlave *i2c, enum i2c_event event)
+static void max7310_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     MAX7310State *s = (MAX7310State *) i2c;
     s->len = 0;
diff --git a/hw/pxa2xx.c b/hw/pxa2xx.c
index e616979..fb1fd0d 100644
--- a/hw/pxa2xx.c
+++ b/hw/pxa2xx.c
@@ -1239,7 +1239,8 @@ static void pxa2xx_i2c_update(PXA2xxI2CState *s)
 }
 
 /* These are only stubs now.  */
-static void pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event)
+static void pxa2xx_i2c_event(I2CSlave *i2c, enum i2c_event event,
+                             uint8_t param)
 {
     PXA2xxI2CSlaveState *slave = FROM_I2C_SLAVE(PXA2xxI2CSlaveState, i2c);
     PXA2xxI2CState *s = slave->host;
diff --git a/hw/smbus.c b/hw/smbus.c
index e3cf6a2..7d070d9 100644
--- a/hw/smbus.c
+++ b/hw/smbus.c
@@ -66,7 +66,7 @@ static void smbus_do_write(SMBusDevice *dev)
     }
 }
 
-static void smbus_i2c_event(I2CSlave *s, enum i2c_event event)
+static void smbus_i2c_event(I2CSlave *s, enum i2c_event event, uint8_t param)
 {
     SMBusDevice *dev = SMBUS_DEVICE(s);
 
diff --git a/hw/ssd0303.c b/hw/ssd0303.c
index d7fd828..19a8d66 100644
--- a/hw/ssd0303.c
+++ b/hw/ssd0303.c
@@ -173,7 +173,7 @@ static int ssd0303_send(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void ssd0303_event(I2CSlave *i2c, enum i2c_event event)
+static void ssd0303_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     ssd0303_state *s = (ssd0303_state *)i2c;
     switch (event) {
diff --git a/hw/tmp105.c b/hw/tmp105.c
index 8e8dbd9..a6dc39a 100644
--- a/hw/tmp105.c
+++ b/hw/tmp105.c
@@ -163,7 +163,7 @@ static int tmp105_tx(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void tmp105_event(I2CSlave *i2c, enum i2c_event event)
+static void tmp105_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     TMP105State *s = (TMP105State *) i2c;
 
diff --git a/hw/tosa.c b/hw/tosa.c
index 512278c..2d2ac54 100644
--- a/hw/tosa.c
+++ b/hw/tosa.c
@@ -157,7 +157,7 @@ static int tosa_dac_send(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void tosa_dac_event(I2CSlave *i2c, enum i2c_event event)
+static void tosa_dac_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     TosaDACState *s = FROM_I2C_SLAVE(TosaDACState, i2c);
     s->len = 0;
diff --git a/hw/twl92230.c b/hw/twl92230.c
index 0d70d84..898d3a1 100644
--- a/hw/twl92230.c
+++ b/hw/twl92230.c
@@ -706,7 +706,7 @@ static void menelaus_write(void *opaque, uint8_t addr, uint8_t value)
     }
 }
 
-static void menelaus_event(I2CSlave *i2c, enum i2c_event event)
+static void menelaus_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     MenelausState *s = (MenelausState *) i2c;
 
diff --git a/hw/wm8750.c b/hw/wm8750.c
index 44f138f..9358342 100644
--- a/hw/wm8750.c
+++ b/hw/wm8750.c
@@ -297,7 +297,7 @@ static void wm8750_reset(I2CSlave *i2c)
     s->i2c_len = 0;
 }
 
-static void wm8750_event(I2CSlave *i2c, enum i2c_event event)
+static void wm8750_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     WM8750State *s = (WM8750State *) i2c;
 
diff --git a/hw/z2.c b/hw/z2.c
index f62b806..ca69f60 100644
--- a/hw/z2.c
+++ b/hw/z2.c
@@ -216,7 +216,7 @@ static int aer915_send(I2CSlave *i2c, uint8_t data)
     return 0;
 }
 
-static void aer915_event(I2CSlave *i2c, enum i2c_event event)
+static void aer915_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
 {
     AER915State *s = FROM_I2C_SLAVE(AER915State, i2c);
     switch (event) {
-- 
1.7.3.4

[Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model

[Jan Kiszka]

This implements I2C EEPROMs of the AT24Cxx series. Sizes from 1Kbit to
1024Kbit are supported. Each EEPROM is backed by a block device. Its
size can be explicitly specified by the "size" property (required for
sizes < 512, the blockdev sector size) or is derived from the size of
the backing block device. Device addresses are built from the device
number property. Write protection can be configured by declaring the
block device read-only.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/Makefile.objs |    2 +-
 hw/at24.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 364 insertions(+), 1 deletions(-)
 create mode 100644 hw/at24.c

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index ea46f81..0c64712 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -170,7 +170,7 @@ common-obj-$(CONFIG_SSD0323) += ssd0323.o
 common-obj-$(CONFIG_ADS7846) += ads7846.o
 common-obj-$(CONFIG_MAX111X) += max111x.o
 common-obj-$(CONFIG_DS1338) += ds1338.o
-common-obj-y += i2c.o smbus.o smbus_eeprom.o
+common-obj-y += i2c.o smbus.o smbus_eeprom.o at24.o
 common-obj-y += eeprom93xx.o
 common-obj-y += scsi-disk.o cdrom.o hd-geometry.o block-common.o
 common-obj-y += scsi-generic.o scsi-bus.o
diff --git a/hw/at24.c b/hw/at24.c
new file mode 100644
index 0000000..e34f2da
--- /dev/null
+++ b/hw/at24.c
@@ -0,0 +1,363 @@
+/*
+ * AT24Cxx EEPROM emulation
+ *
+ * Copyright (c) 2012 Siemens AG
+ * Author: Jan Kiszka
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw.h"
+#include "i2c.h"
+#include "blockdev.h"
+#include "hw/block-common.h"
+
+#define AT24_BASE_ADDRESS   0x50
+#define AT24_MAX_PAGE_LEN   256
+
+typedef enum AT24TransferState {
+    AT24_IDLE,
+    AT24_RD_ADDR,
+    AT24_WR_ADDR_HI,
+    AT24_WR_ADDR_LO,
+    AT24_RW_DATA0,
+    AT24_RD_DATA,
+    AT24_WR_DATA,
+} AT24TransferState;
+
+typedef struct AT24State {
+    I2CSlave i2c;
+    BlockConf block_conf;
+    BlockDriverState *bs;
+    uint32_t size;
+    bool wp;
+    unsigned int addr_mask;
+    unsigned int page_mask;
+    bool addr16;
+    unsigned int hi_addr_mask;
+    uint8_t device;
+    AT24TransferState transfer_state;
+    uint8_t sector_buffer[BDRV_SECTOR_SIZE];
+    int cached_sector;
+    bool cache_dirty;
+    uint32_t transfer_addr;
+} AT24State;
+
+static void at24_flush_transfer_buffer(AT24State *s)
+{
+    if (s->cached_sector < 0 || !s->cache_dirty) {
+        return;
+    }
+    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
+    s->cache_dirty = false;
+}
+
+static void at24_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+
+    switch (event) {
+    case I2C_START_SEND:
+        switch (s->transfer_state) {
+        case AT24_IDLE:
+            if (s->addr16) {
+                s->transfer_addr = (param & s->hi_addr_mask) << 16;
+                s->transfer_state = AT24_WR_ADDR_HI;
+            } else {
+                s->transfer_addr = (param & s->hi_addr_mask) << 8;
+                s->transfer_state = AT24_WR_ADDR_LO;
+            }
+            break;
+        default:
+            s->transfer_state = AT24_IDLE;
+            break;
+        }
+        break;
+    case I2C_START_RECV:
+        switch (s->transfer_state) {
+        case AT24_IDLE:
+            s->transfer_state = AT24_RD_ADDR;
+            break;
+        case AT24_RW_DATA0:
+            s->transfer_state = AT24_RD_DATA;
+            break;
+        default:
+            s->transfer_state = AT24_IDLE;
+            break;
+        }
+        break;
+    case I2C_FINISH:
+        switch (s->transfer_state) {
+        case AT24_WR_DATA:
+            at24_flush_transfer_buffer(s);
+            /* fall through */
+        default:
+            s->transfer_state = AT24_IDLE;
+            break;
+        }
+        break;
+    default:
+        s->transfer_state = AT24_IDLE;
+        break;
+    }
+}
+
+static int at24_cache_sector(AT24State *s, int sector)
+{
+    int ret;
+
+    if (s->cached_sector == sector) {
+        return 0;
+    }
+    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);
+    if (ret < 0) {
+        s->cached_sector = -1;
+        return ret;
+    }
+    s->cached_sector = sector;
+    s->cache_dirty = false;
+    return 0;
+}
+
+static int at24_tx(I2CSlave *i2c, uint8_t data)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+
+    switch (s->transfer_state) {
+    case AT24_WR_ADDR_HI:
+        s->transfer_addr |= (data << 8) & s->addr_mask;
+        s->transfer_state = AT24_WR_ADDR_LO;
+        break;
+    case AT24_WR_ADDR_LO:
+        s->transfer_addr |= data & s->addr_mask;
+        s->transfer_state = AT24_RW_DATA0;
+        break;
+    case AT24_RW_DATA0:
+        s->transfer_state = AT24_WR_DATA;
+        if (at24_cache_sector(s, s->transfer_addr >> BDRV_SECTOR_BITS) < 0 ) {
+            s->transfer_state = AT24_IDLE;
+            return -1;
+        }
+        /* fall through */
+    case AT24_WR_DATA:
+        if (!s->wp) {
+            s->sector_buffer[s->transfer_addr & ~BDRV_SECTOR_MASK] = data;
+            s->cache_dirty = true;
+        }
+        s->transfer_addr = (s->transfer_addr & s->page_mask) |
+            ((s->transfer_addr + 1) & ~s->page_mask);
+        break;
+    default:
+        s->transfer_state = AT24_IDLE;
+        return -1;
+    }
+    return 0;
+}
+
+static int at24_rx(I2CSlave *i2c)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+    unsigned int sector, offset;
+    int result;
+
+    switch (s->transfer_state) {
+    case AT24_RD_ADDR:
+        s->transfer_state = AT24_IDLE;
+        result = s->transfer_addr;
+        break;
+    case AT24_RD_DATA:
+        sector = s->transfer_addr >> BDRV_SECTOR_BITS;
+        offset = s->transfer_addr & ~BDRV_SECTOR_MASK;
+        s->transfer_addr = (s->transfer_addr + 1) & s->addr_mask;
+        if (at24_cache_sector(s, sector) < 0 ) {
+            result = 0;
+            break;
+        }
+        result = s->sector_buffer[offset];
+        break;
+    default:
+        s->transfer_state = AT24_IDLE;
+        result = 0;
+        break;
+    }
+    return result;
+}
+
+static void at24_reset(DeviceState *d)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c.qdev, d);
+
+    s->transfer_state = AT24_IDLE;
+    s->cached_sector = -1;
+}
+
+static int at24_init(I2CSlave *i2c)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+    unsigned int page_size;
+    int64_t image_size;
+    int device_bits;
+    int hi_addr_bits;
+    int dev_no;
+
+    assert(AT24_MAX_PAGE_LEN <= BDRV_SECTOR_SIZE);
+
+    s->bs = s->block_conf.bs;
+    if (!s->bs) {
+        error_report("drive property not set");
+        return -1;
+    }
+
+    s->wp = bdrv_is_read_only(s->bs);
+
+    image_size = bdrv_getlength(s->bs);
+    if (s->size == 0) {
+        s->size = image_size;
+    } else if (image_size < s->size) {
+        error_report("image file smaller than specified EEPROM size");
+        return -1;
+    }
+
+    switch (s->size * 8) {
+    case 1*1024:
+    case 2*1024:
+        page_size = 8;
+        device_bits = 3;
+        hi_addr_bits = 0;
+        break;
+    case 4*1024:
+        page_size = 16;
+        device_bits = 2;
+        hi_addr_bits = 1;
+        break;
+    case 8*1024:
+        page_size = 16;
+        device_bits = 1;
+        hi_addr_bits = 2;
+        break;
+    case 16*1024:
+        page_size = 16;
+        device_bits = 0;
+        hi_addr_bits = 3;
+        break;
+    case 32*1024:
+    case 64*1024:
+        page_size = 32;
+        device_bits = 3;
+        hi_addr_bits = 0;
+        s->addr16 = true;
+        break;
+    case 128*1024:
+    case 256*1024:
+        page_size = 64;
+        device_bits = 2;
+        hi_addr_bits = 0;
+        s->addr16 = true;
+        break;
+    case 512*1024:
+        page_size = 128;
+        device_bits = 2;
+        hi_addr_bits = 0;
+        s->addr16 = true;
+        break;
+    case 1024*1024:
+        page_size = 256;
+        device_bits = 1;
+        hi_addr_bits = 1;
+        s->addr16 = true;
+        break;
+    default:
+        error_report("invalid EEPROM size, must be 2^(10..17)");
+        return -1;
+    }
+    s->addr_mask = s->size - 1;
+    s->page_mask = ~(page_size - 1);
+    s->hi_addr_mask = (1 << hi_addr_bits) - 1;
+
+    dev_no = (s->device & ((1 << device_bits) - 1)) << hi_addr_bits;
+    i2c->address = AT24_BASE_ADDRESS | dev_no;
+    i2c->address_mask &= ~s->hi_addr_mask;
+
+    return 0;
+}
+
+static void at24_pre_save(void *opaque)
+{
+    AT24State *s = opaque;
+
+    at24_flush_transfer_buffer(s);
+}
+
+static int at24_post_load(void *opaque, int version_id)
+{
+    AT24State *s = opaque;
+
+    s->cached_sector = -1;
+    return 0;
+}
+
+static const VMStateDescription vmstate_at24 = {
+    .name = "at24",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .pre_save = at24_pre_save,
+    .post_load = at24_post_load,
+    .fields = (VMStateField []) {
+        VMSTATE_UINT32(transfer_state, AT24State),
+        VMSTATE_UINT32(transfer_addr, AT24State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property at24_properties[] = {
+    DEFINE_BLOCK_PROPERTIES(AT24State, block_conf),
+    DEFINE_PROP_UINT8("device", AT24State, device, 0),
+    DEFINE_PROP_UINT32("size", AT24State, size, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void at24_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+    sc->init = at24_init;
+    sc->event = at24_event;
+    sc->recv = at24_rx;
+    sc->send = at24_tx;
+    dc->desc = "AT24Cxx EEPROM";
+    dc->reset = at24_reset;
+    dc->vmsd = &vmstate_at24;
+    dc->props = at24_properties;
+}
+
+static TypeInfo at24_info = {
+    .name          = "at24",
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(AT24State),
+    .class_init    = at24_class_init,
+};
+
+static void at24_register_types(void)
+{
+    type_register_static(&at24_info);
+}
+
+type_init(at24_register_types)
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model

[Stefan Hajnoczi]

On Mon, Nov 19, 2012 at 03:24:39PM +0100, Jan Kiszka wrote:
> +static void at24_flush_transfer_buffer(AT24State *s)
> +{
> +    if (s->cached_sector < 0 || !s->cache_dirty) {
> +        return;
> +    }
> +    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
[...]
> +static int at24_cache_sector(AT24State *s, int sector)
> +{
> +    int ret;
> +
> +    if (s->cached_sector == sector) {
> +        return 0;
> +    }
> +    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);

Can you use bdrv_aio_writev()/bdrv_aio_readv()?  We should avoid adding
new synchronous block I/O.  Because it forces us to run a nested event
loop that blocks the guest until I/O completes.

Stefan

Re: [Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model

[Jan Kiszka]

On 2012-11-20 14:27, Stefan Hajnoczi wrote:
> On Mon, Nov 19, 2012 at 03:24:39PM +0100, Jan Kiszka wrote:
>> +static void at24_flush_transfer_buffer(AT24State *s)
>> +{
>> +    if (s->cached_sector < 0 || !s->cache_dirty) {
>> +        return;
>> +    }
>> +    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
> [...]
>> +static int at24_cache_sector(AT24State *s, int sector)
>> +{
>> +    int ret;
>> +
>> +    if (s->cached_sector == sector) {
>> +        return 0;
>> +    }
>> +    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);
> 
> Can you use bdrv_aio_writev()/bdrv_aio_readv()?  We should avoid adding
> new synchronous block I/O.  Because it forces us to run a nested event
> loop that blocks the guest until I/O completes.

The call is synchronous as the I2C bus model is as well. How do I model
this with bdrv_aio_*?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model

[Stefan Hajnoczi]

On Tue, Nov 20, 2012 at 02:37:07PM +0100, Jan Kiszka wrote:
> On 2012-11-20 14:27, Stefan Hajnoczi wrote:
> > On Mon, Nov 19, 2012 at 03:24:39PM +0100, Jan Kiszka wrote:
> >> +static void at24_flush_transfer_buffer(AT24State *s)
> >> +{
> >> +    if (s->cached_sector < 0 || !s->cache_dirty) {
> >> +        return;
> >> +    }
> >> +    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
> > [...]
> >> +static int at24_cache_sector(AT24State *s, int sector)
> >> +{
> >> +    int ret;
> >> +
> >> +    if (s->cached_sector == sector) {
> >> +        return 0;
> >> +    }
> >> +    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);
> > 
> > Can you use bdrv_aio_writev()/bdrv_aio_readv()?  We should avoid adding
> > new synchronous block I/O.  Because it forces us to run a nested event
> > loop that blocks the guest until I/O completes.
> 
> The call is synchronous as the I2C bus model is as well. How do I model
> this with bdrv_aio_*?

The bus model needs to be asynchronous.

There are still a few bdrv_read()/bdrv_write() users in hw/ today so
it's not the end of the world if you can't make it async, but it means
someone else will have to do the work eventually :(.

Stefan

Re: [Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model

[Jan Kiszka]

On 2012-11-20 15:06, Stefan Hajnoczi wrote:
> On Tue, Nov 20, 2012 at 02:37:07PM +0100, Jan Kiszka wrote:
>> On 2012-11-20 14:27, Stefan Hajnoczi wrote:
>>> On Mon, Nov 19, 2012 at 03:24:39PM +0100, Jan Kiszka wrote:
>>>> +static void at24_flush_transfer_buffer(AT24State *s)
>>>> +{
>>>> +    if (s->cached_sector < 0 || !s->cache_dirty) {
>>>> +        return;
>>>> +    }
>>>> +    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
>>> [...]
>>>> +static int at24_cache_sector(AT24State *s, int sector)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    if (s->cached_sector == sector) {
>>>> +        return 0;
>>>> +    }
>>>> +    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);
>>>
>>> Can you use bdrv_aio_writev()/bdrv_aio_readv()?  We should avoid adding
>>> new synchronous block I/O.  Because it forces us to run a nested event
>>> loop that blocks the guest until I/O completes.
>>
>> The call is synchronous as the I2C bus model is as well. How do I model
>> this with bdrv_aio_*?
> 
> The bus model needs to be asynchronous.

That's not easy, even in theory. I2C allows a slave to defer its answer
by holding the clock line down, but not all masters support this, and
some (e.g. SMBus) even demand an upper limit. So we risk timeouts of the
guest when deferring normally synchronous accesses like on these EEPROMs.

IOW: There will likely be a need for synchronously waiting on block
layer I/O requests also in the future.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model

[Stefan Hajnoczi]

On Tue, Nov 20, 2012 at 3:38 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2012-11-20 15:06, Stefan Hajnoczi wrote:
>> On Tue, Nov 20, 2012 at 02:37:07PM +0100, Jan Kiszka wrote:
>>> On 2012-11-20 14:27, Stefan Hajnoczi wrote:
>>>> On Mon, Nov 19, 2012 at 03:24:39PM +0100, Jan Kiszka wrote:
>>>>> +static void at24_flush_transfer_buffer(AT24State *s)
>>>>> +{
>>>>> +    if (s->cached_sector < 0 || !s->cache_dirty) {
>>>>> +        return;
>>>>> +    }
>>>>> +    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
>>>> [...]
>>>>> +static int at24_cache_sector(AT24State *s, int sector)
>>>>> +{
>>>>> +    int ret;
>>>>> +
>>>>> +    if (s->cached_sector == sector) {
>>>>> +        return 0;
>>>>> +    }
>>>>> +    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);
>>>>
>>>> Can you use bdrv_aio_writev()/bdrv_aio_readv()?  We should avoid adding
>>>> new synchronous block I/O.  Because it forces us to run a nested event
>>>> loop that blocks the guest until I/O completes.
>>>
>>> The call is synchronous as the I2C bus model is as well. How do I model
>>> this with bdrv_aio_*?
>>
>> The bus model needs to be asynchronous.
>
> That's not easy, even in theory. I2C allows a slave to defer its answer
> by holding the clock line down, but not all masters support this, and
> some (e.g. SMBus) even demand an upper limit. So we risk timeouts of the
> guest when deferring normally synchronous accesses like on these EEPROMs.
>
> IOW: There will likely be a need for synchronously waiting on block
> layer I/O requests also in the future.

How large are the EEPROMs?  Perhaps the data should be in memory?

Stefan

Re: [Qemu-devel] [PATCH 2/2] Add AT24Cxx I2C EEPROM device model

[Jan Kiszka]

On 2012-11-20 15:40, Stefan Hajnoczi wrote:
> On Tue, Nov 20, 2012 at 3:38 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2012-11-20 15:06, Stefan Hajnoczi wrote:
>>> On Tue, Nov 20, 2012 at 02:37:07PM +0100, Jan Kiszka wrote:
>>>> On 2012-11-20 14:27, Stefan Hajnoczi wrote:
>>>>> On Mon, Nov 19, 2012 at 03:24:39PM +0100, Jan Kiszka wrote:
>>>>>> +static void at24_flush_transfer_buffer(AT24State *s)
>>>>>> +{
>>>>>> +    if (s->cached_sector < 0 || !s->cache_dirty) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
>>>>> [...]
>>>>>> +static int at24_cache_sector(AT24State *s, int sector)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (s->cached_sector == sector) {
>>>>>> +        return 0;
>>>>>> +    }
>>>>>> +    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);
>>>>>
>>>>> Can you use bdrv_aio_writev()/bdrv_aio_readv()?  We should avoid adding
>>>>> new synchronous block I/O.  Because it forces us to run a nested event
>>>>> loop that blocks the guest until I/O completes.
>>>>
>>>> The call is synchronous as the I2C bus model is as well. How do I model
>>>> this with bdrv_aio_*?
>>>
>>> The bus model needs to be asynchronous.
>>
>> That's not easy, even in theory. I2C allows a slave to defer its answer
>> by holding the clock line down, but not all masters support this, and
>> some (e.g. SMBus) even demand an upper limit. So we risk timeouts of the
>> guest when deferring normally synchronous accesses like on these EEPROMs.
>>
>> IOW: There will likely be a need for synchronously waiting on block
>> layer I/O requests also in the future.
> 
> How large are the EEPROMs?  Perhaps the data should be in memory?

Up to 128K with these models. But that only solves the read delay.
Writes can be delayed by the EEPROM but have an upper limit for the
completion time (between 5 and 20 ms).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH 2/2 v2] Add AT24Cxx I2C EEPROM device model

[Jan Kiszka]

This implements I2C EEPROMs of the AT24Cxx series. Sizes from 1Kbit to
1024Kbit are supported. Each EEPROM is backed by a block device. Its
size can be explicitly specified by the "size" property (required for
sizes < 512, the blockdev sector size) or is derived from the size of
the backing block device. Device addresses are built from the device
number property. Write protection can be configured by declaring the
block device read-only.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - trivial update to account for blockdev.h moving

 hw/Makefile.objs |    2 +-
 hw/at24.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 364 insertions(+), 1 deletions(-)
 create mode 100644 hw/at24.c

diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 74b07a7..d9394dd 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -176,7 +176,7 @@ common-obj-$(CONFIG_SSD0323) += ssd0323.o
 common-obj-$(CONFIG_ADS7846) += ads7846.o
 common-obj-$(CONFIG_MAX111X) += max111x.o
 common-obj-$(CONFIG_DS1338) += ds1338.o
-common-obj-y += i2c.o smbus.o smbus_eeprom.o
+common-obj-y += i2c.o smbus.o smbus_eeprom.o at24.o
 common-obj-y += eeprom93xx.o
 common-obj-y += scsi-disk.o cdrom.o hd-geometry.o block-common.o
 common-obj-y += scsi-generic.o scsi-bus.o
diff --git a/hw/at24.c b/hw/at24.c
new file mode 100644
index 0000000..c150824
--- /dev/null
+++ b/hw/at24.c
@@ -0,0 +1,363 @@
+/*
+ * AT24Cxx EEPROM emulation
+ *
+ * Copyright (c) 2012 Siemens AG
+ * Author: Jan Kiszka
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "hw.h"
+#include "i2c.h"
+#include "sysemu/blockdev.h"
+#include "hw/block-common.h"
+
+#define AT24_BASE_ADDRESS   0x50
+#define AT24_MAX_PAGE_LEN   256
+
+typedef enum AT24TransferState {
+    AT24_IDLE,
+    AT24_RD_ADDR,
+    AT24_WR_ADDR_HI,
+    AT24_WR_ADDR_LO,
+    AT24_RW_DATA0,
+    AT24_RD_DATA,
+    AT24_WR_DATA,
+} AT24TransferState;
+
+typedef struct AT24State {
+    I2CSlave i2c;
+    BlockConf block_conf;
+    BlockDriverState *bs;
+    uint32_t size;
+    bool wp;
+    unsigned int addr_mask;
+    unsigned int page_mask;
+    bool addr16;
+    unsigned int hi_addr_mask;
+    uint8_t device;
+    AT24TransferState transfer_state;
+    uint8_t sector_buffer[BDRV_SECTOR_SIZE];
+    int cached_sector;
+    bool cache_dirty;
+    uint32_t transfer_addr;
+} AT24State;
+
+static void at24_flush_transfer_buffer(AT24State *s)
+{
+    if (s->cached_sector < 0 || !s->cache_dirty) {
+        return;
+    }
+    bdrv_write(s->bs, s->cached_sector, s->sector_buffer, 1);
+    s->cache_dirty = false;
+}
+
+static void at24_event(I2CSlave *i2c, enum i2c_event event, uint8_t param)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+
+    switch (event) {
+    case I2C_START_SEND:
+        switch (s->transfer_state) {
+        case AT24_IDLE:
+            if (s->addr16) {
+                s->transfer_addr = (param & s->hi_addr_mask) << 16;
+                s->transfer_state = AT24_WR_ADDR_HI;
+            } else {
+                s->transfer_addr = (param & s->hi_addr_mask) << 8;
+                s->transfer_state = AT24_WR_ADDR_LO;
+            }
+            break;
+        default:
+            s->transfer_state = AT24_IDLE;
+            break;
+        }
+        break;
+    case I2C_START_RECV:
+        switch (s->transfer_state) {
+        case AT24_IDLE:
+            s->transfer_state = AT24_RD_ADDR;
+            break;
+        case AT24_RW_DATA0:
+            s->transfer_state = AT24_RD_DATA;
+            break;
+        default:
+            s->transfer_state = AT24_IDLE;
+            break;
+        }
+        break;
+    case I2C_FINISH:
+        switch (s->transfer_state) {
+        case AT24_WR_DATA:
+            at24_flush_transfer_buffer(s);
+            /* fall through */
+        default:
+            s->transfer_state = AT24_IDLE;
+            break;
+        }
+        break;
+    default:
+        s->transfer_state = AT24_IDLE;
+        break;
+    }
+}
+
+static int at24_cache_sector(AT24State *s, int sector)
+{
+    int ret;
+
+    if (s->cached_sector == sector) {
+        return 0;
+    }
+    ret = bdrv_read(s->bs, sector, s->sector_buffer, 1);
+    if (ret < 0) {
+        s->cached_sector = -1;
+        return ret;
+    }
+    s->cached_sector = sector;
+    s->cache_dirty = false;
+    return 0;
+}
+
+static int at24_tx(I2CSlave *i2c, uint8_t data)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+
+    switch (s->transfer_state) {
+    case AT24_WR_ADDR_HI:
+        s->transfer_addr |= (data << 8) & s->addr_mask;
+        s->transfer_state = AT24_WR_ADDR_LO;
+        break;
+    case AT24_WR_ADDR_LO:
+        s->transfer_addr |= data & s->addr_mask;
+        s->transfer_state = AT24_RW_DATA0;
+        break;
+    case AT24_RW_DATA0:
+        s->transfer_state = AT24_WR_DATA;
+        if (at24_cache_sector(s, s->transfer_addr >> BDRV_SECTOR_BITS) < 0 ) {
+            s->transfer_state = AT24_IDLE;
+            return -1;
+        }
+        /* fall through */
+    case AT24_WR_DATA:
+        if (!s->wp) {
+            s->sector_buffer[s->transfer_addr & ~BDRV_SECTOR_MASK] = data;
+            s->cache_dirty = true;
+        }
+        s->transfer_addr = (s->transfer_addr & s->page_mask) |
+            ((s->transfer_addr + 1) & ~s->page_mask);
+        break;
+    default:
+        s->transfer_state = AT24_IDLE;
+        return -1;
+    }
+    return 0;
+}
+
+static int at24_rx(I2CSlave *i2c)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+    unsigned int sector, offset;
+    int result;
+
+    switch (s->transfer_state) {
+    case AT24_RD_ADDR:
+        s->transfer_state = AT24_IDLE;
+        result = s->transfer_addr;
+        break;
+    case AT24_RD_DATA:
+        sector = s->transfer_addr >> BDRV_SECTOR_BITS;
+        offset = s->transfer_addr & ~BDRV_SECTOR_MASK;
+        s->transfer_addr = (s->transfer_addr + 1) & s->addr_mask;
+        if (at24_cache_sector(s, sector) < 0 ) {
+            result = 0;
+            break;
+        }
+        result = s->sector_buffer[offset];
+        break;
+    default:
+        s->transfer_state = AT24_IDLE;
+        result = 0;
+        break;
+    }
+    return result;
+}
+
+static void at24_reset(DeviceState *d)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c.qdev, d);
+
+    s->transfer_state = AT24_IDLE;
+    s->cached_sector = -1;
+}
+
+static int at24_init(I2CSlave *i2c)
+{
+    AT24State *s = DO_UPCAST(AT24State, i2c, i2c);
+    unsigned int page_size;
+    int64_t image_size;
+    int device_bits;
+    int hi_addr_bits;
+    int dev_no;
+
+    assert(AT24_MAX_PAGE_LEN <= BDRV_SECTOR_SIZE);
+
+    s->bs = s->block_conf.bs;
+    if (!s->bs) {
+        error_report("drive property not set");
+        return -1;
+    }
+
+    s->wp = bdrv_is_read_only(s->bs);
+
+    image_size = bdrv_getlength(s->bs);
+    if (s->size == 0) {
+        s->size = image_size;
+    } else if (image_size < s->size) {
+        error_report("image file smaller than specified EEPROM size");
+        return -1;
+    }
+
+    switch (s->size * 8) {
+    case 1*1024:
+    case 2*1024:
+        page_size = 8;
+        device_bits = 3;
+        hi_addr_bits = 0;
+        break;
+    case 4*1024:
+        page_size = 16;
+        device_bits = 2;
+        hi_addr_bits = 1;
+        break;
+    case 8*1024:
+        page_size = 16;
+        device_bits = 1;
+        hi_addr_bits = 2;
+        break;
+    case 16*1024:
+        page_size = 16;
+        device_bits = 0;
+        hi_addr_bits = 3;
+        break;
+    case 32*1024:
+    case 64*1024:
+        page_size = 32;
+        device_bits = 3;
+        hi_addr_bits = 0;
+        s->addr16 = true;
+        break;
+    case 128*1024:
+    case 256*1024:
+        page_size = 64;
+        device_bits = 2;
+        hi_addr_bits = 0;
+        s->addr16 = true;
+        break;
+    case 512*1024:
+        page_size = 128;
+        device_bits = 2;
+        hi_addr_bits = 0;
+        s->addr16 = true;
+        break;
+    case 1024*1024:
+        page_size = 256;
+        device_bits = 1;
+        hi_addr_bits = 1;
+        s->addr16 = true;
+        break;
+    default:
+        error_report("invalid EEPROM size, must be 2^(10..17)");
+        return -1;
+    }
+    s->addr_mask = s->size - 1;
+    s->page_mask = ~(page_size - 1);
+    s->hi_addr_mask = (1 << hi_addr_bits) - 1;
+
+    dev_no = (s->device & ((1 << device_bits) - 1)) << hi_addr_bits;
+    i2c->address = AT24_BASE_ADDRESS | dev_no;
+    i2c->address_mask &= ~s->hi_addr_mask;
+
+    return 0;
+}
+
+static void at24_pre_save(void *opaque)
+{
+    AT24State *s = opaque;
+
+    at24_flush_transfer_buffer(s);
+}
+
+static int at24_post_load(void *opaque, int version_id)
+{
+    AT24State *s = opaque;
+
+    s->cached_sector = -1;
+    return 0;
+}
+
+static const VMStateDescription vmstate_at24 = {
+    .name = "at24",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .pre_save = at24_pre_save,
+    .post_load = at24_post_load,
+    .fields = (VMStateField []) {
+        VMSTATE_UINT32(transfer_state, AT24State),
+        VMSTATE_UINT32(transfer_addr, AT24State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static Property at24_properties[] = {
+    DEFINE_BLOCK_PROPERTIES(AT24State, block_conf),
+    DEFINE_PROP_UINT8("device", AT24State, device, 0),
+    DEFINE_PROP_UINT32("size", AT24State, size, 0),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void at24_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
+
+    sc->init = at24_init;
+    sc->event = at24_event;
+    sc->recv = at24_rx;
+    sc->send = at24_tx;
+    dc->desc = "AT24Cxx EEPROM";
+    dc->reset = at24_reset;
+    dc->vmsd = &vmstate_at24;
+    dc->props = at24_properties;
+}
+
+static TypeInfo at24_info = {
+    .name          = "at24",
+    .parent        = TYPE_I2C_SLAVE,
+    .instance_size = sizeof(AT24State),
+    .class_init    = at24_class_init,
+};
+
+static void at24_register_types(void)
+{
+    type_register_static(&at24_info);
+}
+
+type_init(at24_register_types)
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Stefan Weil]

Am 19.11.2012 15:24, schrieb Jan Kiszka:
> See patches for details.
>
> Jan Kiszka (2):
>    i2c: Introduce device address mask
>    Add AT24Cxx I2C EEPROM device model
>

Is AT24Cxx compatible to Microchip 24C02SC or SGS Thomson ST24C02
(which are emulated in hw/mips_malta.c)? Could we share code?

Stefan

Re: [Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Jan Kiszka]

On 2012-11-19 18:43, Stefan Weil wrote:
> Am 19.11.2012 15:24, schrieb Jan Kiszka:
>> See patches for details.
>>
>> Jan Kiszka (2):
>>    i2c: Introduce device address mask
>>    Add AT24Cxx I2C EEPROM device model
>>
> 
> Is AT24Cxx compatible to Microchip 24C02SC or SGS Thomson ST24C02
> (which are emulated in hw/mips_malta.c)? Could we share code?

Possibly, but definitely not before someone implemented a real I2C
controller model in mips_malta.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Jan Kiszka]

On 2012-11-19 15:24, Jan Kiszka wrote:
> See patches for details.
> 
> Jan Kiszka (2):
>   i2c: Introduce device address mask
>   Add AT24Cxx I2C EEPROM device model
> 
>  hw/Makefile.objs |    2 +-
>  hw/at24.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  hw/ds1338.c      |    2 +-
>  hw/i2c.c         |    9 +-
>  hw/i2c.h         |    3 +-
>  hw/lm832x.c      |    2 +-
>  hw/max7310.c     |    2 +-
>  hw/pxa2xx.c      |    3 +-
>  hw/smbus.c       |    2 +-
>  hw/ssd0303.c     |    2 +-
>  hw/tmp105.c      |    2 +-
>  hw/tosa.c        |    2 +-
>  hw/twl92230.c    |    2 +-
>  hw/wm8750.c      |    2 +-
>  hw/z2.c          |    2 +-
>  15 files changed, 383 insertions(+), 17 deletions(-)
>  create mode 100644 hw/at24.c
> 

Ping. Both still apply over latest master, and addressable review
comments are not pending according to my understanding.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Stefan Hajnoczi]

On Wed, Dec 12, 2012 at 10:44:10AM +0100, Jan Kiszka wrote:
> On 2012-11-19 15:24, Jan Kiszka wrote:
> > See patches for details.
> > 
> > Jan Kiszka (2):
> >   i2c: Introduce device address mask
> >   Add AT24Cxx I2C EEPROM device model
> > 
> >  hw/Makefile.objs |    2 +-
> >  hw/at24.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/ds1338.c      |    2 +-
> >  hw/i2c.c         |    9 +-
> >  hw/i2c.h         |    3 +-
> >  hw/lm832x.c      |    2 +-
> >  hw/max7310.c     |    2 +-
> >  hw/pxa2xx.c      |    3 +-
> >  hw/smbus.c       |    2 +-
> >  hw/ssd0303.c     |    2 +-
> >  hw/tmp105.c      |    2 +-
> >  hw/tosa.c        |    2 +-
> >  hw/twl92230.c    |    2 +-
> >  hw/wm8750.c      |    2 +-
> >  hw/z2.c          |    2 +-
> >  15 files changed, 383 insertions(+), 17 deletions(-)
> >  create mode 100644 hw/at24.c
> > 
> 
> Ping. Both still apply over latest master, and addressable review
> comments are not pending according to my understanding.

Hi Jan,
Just going through my piled up qemu-devel mailbox.  You're asking for a
qemu.git commit to merge this, right?

Stefan

Re: [Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Jan Kiszka]

On 2012-12-18 14:55, Stefan Hajnoczi wrote:
> On Wed, Dec 12, 2012 at 10:44:10AM +0100, Jan Kiszka wrote:
>> On 2012-11-19 15:24, Jan Kiszka wrote:
>>> See patches for details.
>>>
>>> Jan Kiszka (2):
>>>   i2c: Introduce device address mask
>>>   Add AT24Cxx I2C EEPROM device model
>>>
>>>  hw/Makefile.objs |    2 +-
>>>  hw/at24.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/ds1338.c      |    2 +-
>>>  hw/i2c.c         |    9 +-
>>>  hw/i2c.h         |    3 +-
>>>  hw/lm832x.c      |    2 +-
>>>  hw/max7310.c     |    2 +-
>>>  hw/pxa2xx.c      |    3 +-
>>>  hw/smbus.c       |    2 +-
>>>  hw/ssd0303.c     |    2 +-
>>>  hw/tmp105.c      |    2 +-
>>>  hw/tosa.c        |    2 +-
>>>  hw/twl92230.c    |    2 +-
>>>  hw/wm8750.c      |    2 +-
>>>  hw/z2.c          |    2 +-
>>>  15 files changed, 383 insertions(+), 17 deletions(-)
>>>  create mode 100644 hw/at24.c
>>>
>>
>> Ping. Both still apply over latest master, and addressable review
>> comments are not pending according to my understanding.
> 
> Hi Jan,
> Just going through my piled up qemu-devel mailbox.  You're asking for a
> qemu.git commit to merge this, right?

Yes, I'm seeing no blocker for this. I just don't know who feels
responsible for merging things in this domain (I2C).

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Andreas Färber]

Am 12.12.2012 10:44, schrieb Jan Kiszka:
> On 2012-11-19 15:24, Jan Kiszka wrote:
>> See patches for details.
>>
>> Jan Kiszka (2):
>>   i2c: Introduce device address mask
>>   Add AT24Cxx I2C EEPROM device model
>>
>>  hw/Makefile.objs |    2 +-
>>  hw/at24.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  hw/ds1338.c      |    2 +-
>>  hw/i2c.c         |    9 +-
>>  hw/i2c.h         |    3 +-
>>  hw/lm832x.c      |    2 +-
>>  hw/max7310.c     |    2 +-
>>  hw/pxa2xx.c      |    3 +-
>>  hw/smbus.c       |    2 +-
>>  hw/ssd0303.c     |    2 +-
>>  hw/tmp105.c      |    2 +-
>>  hw/tosa.c        |    2 +-
>>  hw/twl92230.c    |    2 +-
>>  hw/wm8750.c      |    2 +-
>>  hw/z2.c          |    2 +-
>>  15 files changed, 383 insertions(+), 17 deletions(-)
>>  create mode 100644 hw/at24.c
>>
> 
> Ping. Both still apply over latest master, and addressable review
> comments are not pending according to my understanding.

Hello Jan, did you see my recent proposal of an I2C qtest framework? :)
For one I'd be interested in feedback from someone who knows I2C and for
another maybe you could follow-up your patch with some test case (I2C
master shouldn't matter, I guess).

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 0/2] i2c: Add AT24Cxx EEPROM model

[Jan Kiszka]

On 2012-12-18 15:26, Andreas Färber wrote:
> Am 12.12.2012 10:44, schrieb Jan Kiszka:
>> On 2012-11-19 15:24, Jan Kiszka wrote:
>>> See patches for details.
>>>
>>> Jan Kiszka (2):
>>>   i2c: Introduce device address mask
>>>   Add AT24Cxx I2C EEPROM device model
>>>
>>>  hw/Makefile.objs |    2 +-
>>>  hw/at24.c        |  363 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  hw/ds1338.c      |    2 +-
>>>  hw/i2c.c         |    9 +-
>>>  hw/i2c.h         |    3 +-
>>>  hw/lm832x.c      |    2 +-
>>>  hw/max7310.c     |    2 +-
>>>  hw/pxa2xx.c      |    3 +-
>>>  hw/smbus.c       |    2 +-
>>>  hw/ssd0303.c     |    2 +-
>>>  hw/tmp105.c      |    2 +-
>>>  hw/tosa.c        |    2 +-
>>>  hw/twl92230.c    |    2 +-
>>>  hw/wm8750.c      |    2 +-
>>>  hw/z2.c          |    2 +-
>>>  15 files changed, 383 insertions(+), 17 deletions(-)
>>>  create mode 100644 hw/at24.c
>>>
>>
>> Ping. Both still apply over latest master, and addressable review
>> comments are not pending according to my understanding.
> 
> Hello Jan, did you see my recent proposal of an I2C qtest framework? :)
> For one I'd be interested in feedback from someone who knows I2C and for
> another maybe you could follow-up your patch with some test case (I2C
> master shouldn't matter, I guess).

I haven't seen your test framework yet, and I'm unfortunately lacking
time to work on test cases ATM. We were simply testing this device
against the Linux driver. Wasn't there also some test environment that
builds on top of a guest OS?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux