Commit c42edde5 authored by Jarkko Nikula's avatar Jarkko Nikula Committed by Wolfram Sang
Browse files

i2c: designware: Fix slave state machine for sequential reads



Some read types from I2C bus don't work correctly when testing the
i2c-designware-slave.c with the slave-eeprom backend. The same reads
work correctly when testing with a real 24c02 EEPROM chip.

In the following tests an i2c-designware-slave.c instance with the
slave-eeprom backend is configured to act as a simulated 24c02 at
address 0x65 on an I2C host bus 6:

1. i2cdump -y 6 0x65 b (OK)
   Random read. Each byte are read using a byte address write with a
   current address read in a same message.
2. i2cdump -y 6 0x65 c (OK, was NOK before commit 3b5f7f10 when it
                        was repeating the 1st byte)
   Repeated current address read. One byte address write message
   followed by repeated current address read messages.
3. i2cdump -y 6 0x65 i (NOK, each 32 byte block repeats the 1st byte of
                        block)
   Sequential read using SMBus Block Read. For each 32 byte block a byte
   address write followed by 32 sequental reads in a same message.

These findings are explained because the implementation has had a
mismatch between hardware interrupts and what I2C slave events should be
sent after those interrupts. Despite that the case 1 happened to have
always the I2C slave events sent to a right order with a right data
between backend and the I2C bus.

Hardware generates the DW_IC_INTR_RD_REQ interrupt when another host is
attempting to read and for sequential reads after. DW_IC_INTR_RX_DONE
occurs when host does not acknowledge a transmitted byte which is an
indication the end of transmission.

Those interrupts do not match directly with I2C_SLAVE_READ_REQUESTED and
I2C_SLAVE_READ_PROCESSED events which is how the code was and is
practically using them. The slave-eeprom backend increases the buffer
index with the I2C_SLAVE_READ_PROCESSED event and returns the data from
current index when receiving only the I2C_SLAVE_READ_REQUESTED event.

That explains the repeated bytes in case 3 and also case 2 before
commit 3b5f7f10 ("i2c: designware: slave should do WRITE_REQUESTED
before WRITE_RECEIVED").

Patch fixes the case 3 while keep cases 1 and 2 working with following
changes:

- First DW_IC_INTR_RD_REQ interrupt will change the state machine to
  read in progress state, send I2C_SLAVE_READ_REQUESTED event and
  transmit the first byte from backend
- Subsequent DW_IC_INTR_RD_REQ interrupts will send
  I2C_SLAVE_READ_PROCESSED events and transmit next bytes from backend
- STOP won't change the state machine. Otherwise case 2 won't work since
  we cannot distinguish current address read from sequentiel read
- DW_IC_INTR_RX_DONE interrupt is needless since there is no mechanism
  to inform it to a backend. It cannot be used to change state machine
  at the end of read either due the same reason than above
- Next host write to us will change the state machine from read to write
  in progress state
- STATUS_WRITE_IN_PROGRESS and STATUS_READ_IN_PROGRESS are considered
  now to be status flags not the state of the driver. This is how we
  treat them in i2c-designware-master.c

While at it do not test the return code from i2c_slave_event() for
I2C_SLAVE_READ_REQUESTED and I2C_SLAVE_READ_PROCESSED since it returns
always 0.

Signed-off-by: default avatarJarkko Nikula <jarkko.nikula@linux.intel.com>
Reviewed-by: default avatarAndy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: default avatarWolfram Sang <wsa@kernel.org>
parent e77f7ba7
Loading
Loading
Loading
Loading
+0 −1
Original line number Diff line number Diff line
@@ -103,7 +103,6 @@
#define DW_IC_INTR_MASTER_MASK		(DW_IC_INTR_DEFAULT_MASK | \
					 DW_IC_INTR_TX_EMPTY)
#define DW_IC_INTR_SLAVE_MASK		(DW_IC_INTR_DEFAULT_MASK | \
					 DW_IC_INTR_RX_DONE | \
					 DW_IC_INTR_RX_UNDER | \
					 DW_IC_INTR_RD_REQ)

+16 −16
Original line number Diff line number Diff line
@@ -173,8 +173,9 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
		enabled, slave_activity, raw_stat, stat);

	if (stat & DW_IC_INTR_RX_FULL) {
		if (dev->status != STATUS_WRITE_IN_PROGRESS) {
			dev->status = STATUS_WRITE_IN_PROGRESS;
		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
			dev->status |= STATUS_WRITE_IN_PROGRESS;
			dev->status &= ~STATUS_READ_IN_PROGRESS;
			i2c_slave_event(dev->slave, I2C_SLAVE_WRITE_REQUESTED,
					&val);
		}
@@ -190,24 +191,23 @@ static int i2c_dw_irq_handler_slave(struct dw_i2c_dev *dev)
		if (slave_activity) {
			regmap_read(dev->map, DW_IC_CLR_RD_REQ, &tmp);

			dev->status = STATUS_READ_IN_PROGRESS;
			if (!i2c_slave_event(dev->slave,
			if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
				i2c_slave_event(dev->slave,
						I2C_SLAVE_READ_REQUESTED,
					     &val))
				regmap_write(dev->map, DW_IC_DATA_CMD, val);
						&val);
				dev->status |= STATUS_READ_IN_PROGRESS;
				dev->status &= ~STATUS_WRITE_IN_PROGRESS;
			} else {
				i2c_slave_event(dev->slave,
						I2C_SLAVE_READ_PROCESSED,
						&val);
			}
			regmap_write(dev->map, DW_IC_DATA_CMD, val);
		}

	if (stat & DW_IC_INTR_RX_DONE) {
		if (!i2c_slave_event(dev->slave, I2C_SLAVE_READ_PROCESSED,
				     &val))
			regmap_read(dev->map, DW_IC_CLR_RX_DONE, &tmp);
	}

	if (stat & DW_IC_INTR_STOP_DET) {
		dev->status = STATUS_IDLE;
	if (stat & DW_IC_INTR_STOP_DET)
		i2c_slave_event(dev->slave, I2C_SLAVE_STOP, &val);
	}

	return 1;
}