Re: [Qemu-devel] [PATCH] i2c: Fix SMBus read transactions to avoid double events

[Corey Minyard <cminyard@mvista.com>]

I just realized that this won't work for true I2C devices.  When simulating
an SMBus transaction on top of I2C, it will start the transaction twice
without an end.  So something else will need to be done.

-corey

On 06/28/2016 11:21 AM, Corey Minyard wrote:
> On 06/27/2016 07:43 PM, Alistair Francis wrote:
>> On Mon, Jun 27, 2016 at 3:04 PM, <minyard@acm.org> wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> Change 2293c27faddf (i2c: implement broadcast write) added broadcast
>>> capability to the I2C bus, but it broke SMBus read transactions.
>>> An SMBus read transaction does two i2c_start_transaction() calls
>>> without an intervening i2c_end_transfer() call.  This will
>>> result in i2c_start_transfer() adding the same device to the
>>> current_devs list twice, and then the ->event() for the same
>>> device gets called twice in the second call to i2c_start_transfer(),
>>> resulting in the smbus code getting confused.
>>>
>>> This fix adds a third state to the i2c_start_transfer() recv
>>> parameter, a read continued that will not scan for devices
>>> and add them to current_devs.  It also adds #defines for all
>>> the values for the recv parameter.
>>>
>>> This also deletes the empty check from the top of i2c_end_transfer().
>>> It's unnecessary, and it prevents the broadcast variable from being
>>> set to false at the end of the transaction if no devices were on
>>> the bus.
>>>
>>> Cc: KONRAD Frederic <fred.konrad@greensocs.com>
>>> Cc: Alistair Francis <alistair.francis@xilinx.com>
>>> Cc: Peter Crosthwaite <crosthwaite.peter@gmail.com>
>>> Cc: Kwon <hyun.kwon@xilinx.com>
>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>>> ---
>>>   hw/i2c/core.c        | 24 +++++++++++-------------
>>>   hw/i2c/smbus.c       | 22 +++++++++++-----------
>>>   include/hw/i2c/i2c.h |  9 +++++++++
>>>   3 files changed, 31 insertions(+), 24 deletions(-)
>>>
>>> I considered a couple of ways to do this.  I thought about adding a
>>> separate function to do a "intermediate end" of the transaction, but
>>> that seemed like too much work.  I also thought about adding a
>>> bool saing whether you are currently in a transaction and not rescan
>>> the bus if you are.  However, that would require that the bool be in
>>> the vmstate, and that would be complicated.
>>>
>>> On that note, the current_devs list is not in the vmstate. That means
>>> that a migrate done in the middle of an I2C transaction will cause the
>>> I2C transaction to fail, right?  Maybe this whole broadcast thing is
>>> a bad idea, or needs a different implementation?
>
> Looking at it a little closer, migration does appear to be handled
> correctly.  So I'll stick with this patch.
>
>>> diff --git a/hw/i2c/core.c b/hw/i2c/core.c
>>> index abb3efb..53069dd 100644
>>> --- a/hw/i2c/core.c
>>> +++ b/hw/i2c/core.c
>>> @@ -101,15 +101,17 @@ int i2c_start_transfer(I2CBus *bus, uint8_t 
>>> address, int recv)
>>>           bus->broadcast = true;
>>>       }
>>>
>>> -    QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>>> -        DeviceState *qdev = kid->child;
>>> -        I2CSlave *candidate = I2C_SLAVE(qdev);
>>> -        if ((candidate->address == address) || (bus->broadcast)) {
>>> -            node = g_malloc(sizeof(struct I2CNode));
>>> -            node->elt = candidate;
>>> -            QLIST_INSERT_HEAD(&bus->current_devs, node, next);
>>> -            if (!bus->broadcast) {
>>> -                break;
>>> +    if (recv != I2C_START_CONTINUED_READ_TRANSFER) {
>>> +        QTAILQ_FOREACH(kid, &bus->qbus.children, sibling) {
>>> +            DeviceState *qdev = kid->child;
>>> +            I2CSlave *candidate = I2C_SLAVE(qdev);
>>> +            if ((candidate->address == address) || (bus->broadcast)) {
>>> +                node = g_malloc(sizeof(struct I2CNode));
>>> +                node->elt = candidate;
>>> +                QLIST_INSERT_HEAD(&bus->current_devs, node, next);
>>> +                if (!bus->broadcast) {
>>> +                    break;
>>> +                }
>>>               }
>>>           }
>>>       }
>>> @@ -134,10 +136,6 @@ void i2c_end_transfer(I2CBus *bus)
>>>       I2CSlaveClass *sc;
>>>       I2CNode *node, *next;
>>>
>>> -    if (QLIST_EMPTY(&bus->current_devs)) {
>>> -        return;
>>> -    }
>>> -
>>>       QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) {
>>>           sc = I2C_SLAVE_GET_CLASS(node->elt);
>>>           if (sc->event) {
>>> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
>>> index 3979b3d..f63799d 100644
>>> --- a/hw/i2c/smbus.c
>>> +++ b/hw/i2c/smbus.c
>>> @@ -222,7 +222,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
>>>   {
>>>       uint8_t data;
>>>
>>> -    if (i2c_start_transfer(bus, addr, 1)) {
>>> +    if (i2c_start_transfer(bus, addr, I2C_START_READ_TRANSFER)) {
>>>           return -1;
>>>       }
>>>       data = i2c_recv(bus);
>>> @@ -233,7 +233,7 @@ int smbus_receive_byte(I2CBus *bus, uint8_t addr)
>>>
>>>   int smbus_send_byte(I2CBus *bus, uint8_t addr, uint8_t data)
>>>   {
>>> -    if (i2c_start_transfer(bus, addr, 0)) {
>>> +    if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
>>>           return -1;
>>>       }
>>>       i2c_send(bus, data);
>>> @@ -244,11 +244,11 @@ int smbus_send_byte(I2CBus *bus, uint8_t addr, 
>>> uint8_t data)
>>>   int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command)
>>>   {
>>>       uint8_t data;
>>> -    if (i2c_start_transfer(bus, addr, 0)) {
>>> +    if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
>>>           return -1;
>>>       }
>>>       i2c_send(bus, command);
>>> -    i2c_start_transfer(bus, addr, 1);
>>> +    i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER);
>>>       data = i2c_recv(bus);
>>>       i2c_nack(bus);
>>>       i2c_end_transfer(bus);
>>> @@ -257,7 +257,7 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, 
>>> uint8_t command)
>>>
>>>   int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, 
>>> uint8_t data)
>>>   {
>>> -    if (i2c_start_transfer(bus, addr, 0)) {
>>> +    if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
>>>           return -1;
>>>       }
>>>       i2c_send(bus, command);
>>> @@ -269,11 +269,11 @@ int smbus_write_byte(I2CBus *bus, uint8_t 
>>> addr, uint8_t command, uint8_t data)
>>>   int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command)
>>>   {
>>>       uint16_t data;
>>> -    if (i2c_start_transfer(bus, addr, 0)) {
>>> +    if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
>>>           return -1;
>>>       }
>>>       i2c_send(bus, command);
>>> -    i2c_start_transfer(bus, addr, 1);
>>> +    i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER);
>>>       data = i2c_recv(bus);
>>>       data |= i2c_recv(bus) << 8;
>>>       i2c_nack(bus);
>>> @@ -283,7 +283,7 @@ int smbus_read_word(I2CBus *bus, uint8_t addr, 
>>> uint8_t command)
>>>
>>>   int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, 
>>> uint16_t data)
>>>   {
>>> -    if (i2c_start_transfer(bus, addr, 0)) {
>>> +    if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
>>>           return -1;
>>>       }
>>>       i2c_send(bus, command);
>>> @@ -298,11 +298,11 @@ int smbus_read_block(I2CBus *bus, uint8_t 
>>> addr, uint8_t command, uint8_t *data)
>>>       int len;
>>>       int i;
>>>
>>> -    if (i2c_start_transfer(bus, addr, 0)) {
>>> +    if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
>>>           return -1;
>>>       }
>>>       i2c_send(bus, command);
>>> -    i2c_start_transfer(bus, addr, 1);
>>> +    i2c_start_transfer(bus, addr, I2C_START_CONTINUED_READ_TRANSFER);
>>>       len = i2c_recv(bus);
>>>       if (len > 32) {
>>>           len = 0;
>>> @@ -323,7 +323,7 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, 
>>> uint8_t command, uint8_t *data,
>>>       if (len > 32)
>>>           len = 32;
>>>
>>> -    if (i2c_start_transfer(bus, addr, 0)) {
>>> +    if (i2c_start_transfer(bus, addr, I2C_START_WRITE_TRANSFER)) {
>>>           return -1;
>>>       }
>>>       i2c_send(bus, command);
>>> diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h
>>> index c4085aa..16c910e 100644
>>> --- a/include/hw/i2c/i2c.h
>>> +++ b/include/hw/i2c/i2c.h
>>> @@ -50,6 +50,15 @@ struct I2CSlave
>>>       uint8_t address;
>>>   };
>>>
>>> +/* For the recv value in i2c_start_transfer.  The first two values
>>> +   correspond to false and true for the recv value.  The third is a
>>> +   special value that is used to tell i2c_start_transfer that this is
>>> +   a continuation of the previous transfer, so don't rescan the bus
>>> +   for devices to send to, continue with the current set of 
>>> devices. */
>> This comment is a little confusing, I don't think you need to explain
>> what the read/write values correspond to but you clear up the
>> explanation about the continued read value. Something like this I like
>> is clearer:
>>
>> The continued read is a special value that is used to tell the
>> i2c_start_transfer() function that this is a continuation of the
>> previous transfer so we don't rescan the bus for devices to send to
>> and instead just continue with the current set of devices.
>
> I did think about doing it this way, but it seemed clearer to me the
> other way.  But I think you are right.
>
> -corey
>
>> Otherwise:
>> Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>
>>
>> Thanks,
>>
>> Alistair
>>
>>
>>> +#define I2C_START_WRITE_TRANSFER          0
>>> +#define I2C_START_READ_TRANSFER           1
>>> +#define I2C_START_CONTINUED_READ_TRANSFER 2
>>> +
>>>   I2CBus *i2c_init_bus(DeviceState *parent, const char *name);
>>>   void i2c_set_slave_address(I2CSlave *dev, uint8_t address);
>>>   int i2c_bus_busy(I2CBus *bus);
>>> -- 
>>> 2.7.4
>>>
>>>
>