> > +#include "hw.h"
> > +#include "i2c.h"
>
> Please use "hw/hw.h" and "hw/i2c.h" since Paolo is planning to move
> I2C devices into hw/i2c/.
No, I2C _masters_ move into hw/i2c. This would probably move into
hw/nvram.
Paolo
> > +#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;
>
> parent_obj and separate from remaining fields please. All uses of
> this
> field except in VMState will be wrong.
>
> http://wiki.qemu.org/QOMConventions
>
> > + 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);
>
> Please don't use DO_UPCAST(), add a QOM cast macro AT24() instead.
>
> > +
> > + 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)
>
> hw/i2c.c:i2c_slave_qdev_init() calls I2CSlaveClass::init only, so
> please
> use a realize function instead. Cf. hw/qdev-core.h.
>
> > +{
> > + 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);
>
> This should instead do error_setg(errp, "...") then.
>
> > +
> > + s->bs = s->block_conf.bs;
> > + if (!s->bs) {
> > + error_report("drive property not set");
>
> Dito, same below.
>
> > + 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 = {
>
> http://git.qemu.org/?p=qemu.git;a=commit;h=8c43a6f05d5ef3c9484bd2be9d4e818d58e62016
> updated all TypeInfos to be static const, please don't regress.
>
> > + .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)
>
> My I2C libqos has landed in qemu.git a while ago, so this new device
> might be a candidate for a qtest case now, no? The easiest way I see
> would be arm with -M n810 -device at24,device=x,size=y since OMAP has
> an
> I2C libqos implementation and has same endianness as x86.
>
> Regards,
> Andreas
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG
> Nürnberg
>
