qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Corey Minyard
Subject: Re: [Qemu-devel] [PATCH] i2c: Fix SMBus read transactions to avoid double events
Date: Tue, 28 Jun 2016 12:45:31 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 
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, <address@hidden> wrote:

From: Corey Minyard <address@hidden>

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 <address@hidden>
Cc: Alistair Francis <address@hidden>
Cc: Peter Crosthwaite <address@hidden>
Cc: Kwon <address@hidden>
Cc: Peter Maydell <address@hidden>
Signed-off-by: Corey Minyard <address@hidden>
---
  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 <address@hidden>

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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]