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

[Jan Kiszka]

On 2013-02-27 05:44, Peter Crosthwaite wrote:
> Hi Jan,
> 
> I actually have my own subset implementation of this currently on
> list. Yours is more complete however, So id prefer to drop mine in
> favour of yours and reverify Zynq as working with yours.

Great! I'm trying to look for a slot to address the review comments and
come up with a new version.

> 
> On Sat, Feb 23, 2013 at 6:39 AM, Jan Kiszka <address@hidden> wrote:
>> 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 <address@hidden>
>> ---
>>  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 a1f3a80..a64700c 100644
>> --- a/hw/Makefile.objs
>> +++ b/hw/Makefile.objs
>> @@ -178,7 +178,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..0dfefd4
>> --- /dev/null
>> +++ b/hw/at24.c
>> @@ -0,0 +1,363 @@
>> +/*
>> + * AT24Cxx EEPROM emulation
>> + *
>> + * Copyright (c) Siemens AG, 2012
>> + * 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:
> 
> This looks like a guest error. Could add a
> qemu_log_mask(LOG_GUEST_ERROR for completeness. Same for other
> default-back-to-idle conditions below.

OK.

> 
>> +            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;
>> +        }
> 
> Minor nitpick - theres an assumption here that page_size <
> BDRV_SECTOR_SIZE. The device will bug if larger page devices are added
> in the future.

Yes, there is an assert (which should better be a BUILD_BUG_ON) below. I
don't think it's worth to make the code more clever than that.

> 
>> +        /* 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;
> 
> Can we data drive this in some way? Pull this out into a table of some
> description.

Sure, will play with it.

> 
>> +    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;
> 
> This kinda breaks the I2C address property. Machine models are able to
> set I2C addresses and this code here is trampling it.

Well, it's implementing the device spec.

> I think it would
> be cleaner to generalise the I2CSlave::address property to handle the
> device_bits concept as you have introduced here. Many I2C devices have
> the device_bits concept not just AT24. E.G. I2C slaves would then have
> three props
> 
> num_address_bits (here as device_bits) -> number of high order address
> bits the machine model can set (in most cases these are physical pins
> on the device).
> address -> value of the high order address bits
> address_mask -> same as implmented in this series
> 
> num_address_bits defaults to 7 for backwards compatibility of existing
> I2C devices. Anyone who sets an I2C address without changing
> num_address_bits just sets a full 7 bit I2C address. Anyone who
> changes num_address_bits will then interpret the i2c address as these
> device_bits.

Hmm, will think about this. But I'm not a fan of making something
(user-)configurable that isn't - num_address_bits/device_bits are
hard-coded values the device defines.

> 
>> +    i2c->address_mask &= ~s->hi_addr_mask;
>> +
> 
> Both address and address_mask are private internals to I2C_SLAVE. I
> think they should be encapsulated by setters. i2c_set_slave_address()
> already exists. Just need to add its new buddy, i2c_set_slave_mask().

OK, that's simple.

Thanks,
Jan

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