On Mon, Jun 27, 2016 at 3:04 PM, <miny...@acm.org> wrote:
> From: Corey Minyard <cminy...@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.kon...@greensocs.com>
> Cc: Alistair Francis <alistair.fran...@xilinx.com>
> Cc: Peter Crosthwaite <crosthwaite.pe...@gmail.com>
> Cc: Kwon <hyun.k...@xilinx.com>
> Cc: Peter Maydell <peter.mayd...@linaro.org>
> Signed-off-by: Corey Minyard <cminy...@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?
>
> 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.
Otherwise:
Reviewed-by: Alistair Francis <alistair.fran...@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
>
>
