From patchwork Mon Jun 27 22:04:25 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Corey Minyard X-Patchwork-Id: 70962 Delivered-To: patch@linaro.org Received: by 10.140.28.4 with SMTP id 4csp1277764qgy; Mon, 27 Jun 2016 15:05:53 -0700 (PDT) X-Received: by 10.55.36.202 with SMTP id k71mr25668437qkk.92.1467065153352; Mon, 27 Jun 2016 15:05:53 -0700 (PDT) Return-Path: Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id r3si19175271qkc.200.2016.06.27.15.05.53 for (version=TLS1 cipher=AES128-SHA bits=128/128); Mon, 27 Jun 2016 15:05:53 -0700 (PDT) Received-SPF: pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Authentication-Results: mx.google.com; dkim=fail header.i=@gmail.com; spf=pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom=qemu-devel-bounces+patch=linaro.org@nongnu.org Received: from localhost ([::1]:33108 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHefA-0004Yh-Ui for patch@linaro.org; Mon, 27 Jun 2016 18:05:52 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46060) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHedx-00046I-Hw for qemu-devel@nongnu.org; Mon, 27 Jun 2016 18:04:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bHedt-00086j-0D for qemu-devel@nongnu.org; Mon, 27 Jun 2016 18:04:36 -0400 Received: from mail-pa0-x244.google.com ([2607:f8b0:400e:c03::244]:35742) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bHeds-00085x-Lm for qemu-devel@nongnu.org; Mon, 27 Jun 2016 18:04:32 -0400 Received: by mail-pa0-x244.google.com with SMTP id hf6so16351713pac.2 for ; Mon, 27 Jun 2016 15:04:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:date:message-id; bh=iF2Fnbn8+ACXMHSJMQQiboI8XOdK6CRq4tu6Q56gfnc=; b=Q/X0GoNfI5o/yEyljj4n1Q4pgop85ziMh9KWZmp6s9xzkjthPkYsfC4/WOLfIoP/mD N1XRlTBx38HmJgqSB6Db2+Thj9Fx7Wi6VR8d1DlUhEKroRGMLpm3MBKthu/UKu6ex3DA BI1f3q/C9fC+WxZkKrhSwhesvpkpZmZ4C0jcYoCFRIjqASMP8KFgKlxTe8CNuDiBf2Fy Eh25wF13f/FmsVyk4etSrBCcNzVBaZSMHsiaVFTFkZHN7RTXsoGR57tFzi2IDS9s+l3M Q2tvpw639SviTgq7CoylHmbgSmErzBezbnxwnIbZlP8rIvfNeEoPpcf/rzyHBQJvALha HFIw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:date:message-id; bh=iF2Fnbn8+ACXMHSJMQQiboI8XOdK6CRq4tu6Q56gfnc=; b=KMshvKKC599rtdooJmVAkn0R4Q9BUhOi9fcZijTPnIM6ecQZYmlK68QYrDY+RIbyo9 ZcKNXExustpz6b/uXJjQSwYN57cxdzAGVSPMITeEXP/ZZERIu3Sf9nWJevIUKtue1kKM 7952oZqEOqDrje8aSurKqGVykF9VyoSx8acf3lDZae7GNRtdXGrxWcUL/gbh9DhYMBFP Bxjg/vizlrqBRq/HEcIHnrVGTPopH3jI/vNtPI0lXvZCWuFYmDB5W1yqYTsQ41aXxEN7 bjqEdGVBFWW5Dd7FWDiZl3twsoa+73MaYiwKkvtfh5u11liF3gsUMXreQdAbbA/NiA98 oFuw== X-Gm-Message-State: ALyK8tLGzDYgj5jt2ASQeXCdz273/QZXnCO6EcBvznldEyBkwFZlEt5tLr25xFnKofcrJw== X-Received: by 10.66.67.1 with SMTP id j1mr37425237pat.57.1467065071565; Mon, 27 Jun 2016 15:04:31 -0700 (PDT) Received: from serve.minyard.net ([173.64.235.97]) by smtp.gmail.com with ESMTPSA id 186sm1509804pfg.39.2016.06.27.15.04.30 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 27 Jun 2016 15:04:30 -0700 (PDT) Received: from t430.minyard.net (unknown [IPv6:2001:470:b8f6:1b:55c7:e79f:7b52:ac04]) by serve.minyard.net (Postfix) with ESMTPA id 3910C6CF; Mon, 27 Jun 2016 17:04:29 -0500 (CDT) Received: by t430.minyard.net (Postfix, from userid 1000) id 506CA300550; Mon, 27 Jun 2016 17:04:28 -0500 (CDT) From: minyard@acm.org To: qemu-devel@nongnu.org, minyard@acm.org, cminyard@mvista.com Date: Mon, 27 Jun 2016 17:04:25 -0500 Message-Id: <1467065065-3980-1-git-send-email-minyard@acm.org> X-Mailer: git-send-email 2.7.4 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2607:f8b0:400e:c03::244 Subject: [Qemu-devel] [PATCH] i2c: Fix SMBus read transactions to avoid double events X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Peter Maydell , Peter Crosthwaite , Alistair Francis , Kwon , KONRAD Frederic Errors-To: qemu-devel-bounces+patch=linaro.org@nongnu.org Sender: "Qemu-devel" From: Corey Minyard 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 Cc: Alistair Francis Cc: Peter Crosthwaite Cc: Kwon Cc: Peter Maydell Signed-off-by: Corey Minyard --- 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? -- 2.7.4 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. */ +#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);