I2C_M_RECV_LEN for i2c-mxs

Hello,

I'm currently trying to teach the i2c-mxs driver to handle the I2C_M_RECV_LEN flag but failed up to now. Maybe someone has an good idea how to make it running?

My current patch looks as follows:

@@ -1,3 +1,4 @@
+#define DEBUG
 /*
  * Freescale MXS I2C bus driver
  *

@@ -289,14 +290,23 @@ write_init_pio_fail:
 static int mxs_i2c_pio_wait_dmareq(struct mxs_i2c_dev *i2c)
 {
 	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
+	u32 reg, reg_old;
+	int first = 1;
 
-	while (!(readl(i2c->regs + MXS_I2C_DEBUG0) &
+	while (!(reg = readl(i2c->regs + MXS_I2C_DEBUG0) &
 		MXS_I2C_DEBUG0_DMAREQ)) {
+		if (first || reg != reg_old) {
+			pr_debug("%s: I2C_DEBUG = 0x%08x\n", __func__, reg);
+			first = 0;
+			reg_old = reg;
+		}
 		if (time_after(jiffies, timeout))
 			return -ETIMEDOUT;
 		cond_resched();
 	}
 
+	pr_debug("%s: I2C_DEBUG = 0x%08x\n", __func__, reg);
+
 	writel(MXS_I2C_DEBUG0_DMAREQ, i2c->regs + MXS_I2C_DEBUG0_CLR);
 
 	return 0;

@@ -338,6 +348,9 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_CLR);
 
 	if (msg->flags & I2C_M_RD) {
+		int readlen = msg->len;
+		size_t buf_offset = 0;
+
 		addr_data |= I2C_SMBUS_READ;
 
 		/* SELECT command. */

@@ -354,19 +367,39 @@ static int mxs_i2c_pio_setup_xfer(struct i2c_adapter *adap,
 		if (ret)
 			return ret;
 
+		if (msg->flags & I2C_M_RECV_LEN) {
+			pr_debug("read first byte\n");
+			writel(MXS_I2C_CTRL0_RUN | MXS_I2C_CTRL0_MASTER_MODE |
+				MXS_I2C_CTRL0_XFER_COUNT(1),
+				i2c->regs + MXS_I2C_CTRL0);
+
+			ret = mxs_i2c_pio_wait_dmareq(i2c);
+			if (ret)
+				return ret;
+
+			data = readl(i2c->regs + MXS_I2C_DATA) & 0xff;
+			if (data > I2C_SMBUS_BLOCK_MAX)
+				return -EPROTO;
+
+			msg->len += data;
+			readlen += data - 1;
+		}
+
+		pr_debug("%s: data = %u, msg->len = %u, readlen = %u\n", __func__, data, msg->len, readlen);
+
 		/* READ command. */
 		writel(MXS_I2C_CTRL0_RUN | MXS_CMD_I2C_READ | flags |
-			MXS_I2C_CTRL0_XFER_COUNT(msg->len),
+			MXS_I2C_CTRL0_XFER_COUNT(readlen),
 			i2c->regs + MXS_I2C_CTRL0);
 
-		for (i = 0; i < msg->len; i++) {
+		for (i = 0; i < readlen; i++) {
 			if ((i & 3) == 0) {
 				ret = mxs_i2c_pio_wait_dmareq(i2c);
 				if (ret)
 					return ret;
 				data = readl(i2c->regs + MXS_I2C_DATA);
 			}
-			msg->buf[i] = data & 0xff;
+			msg->buf[buf_offset + i] = data & 0xff;
 			data >>= 8;
 		}
 	} else {

@@ -438,6 +471,9 @@ static int mxs_i2c_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg,
 	 * is set to 8 bytes, transfers shorter than 8 bytes are transfered
 	 * using PIO mode while longer transfers use DMA. The 8 byte border is
 	 * based on this empirical measurement and a lot of previous frobbing.
+	 *
+	 * Note that only mxs_i2c_pio_setup_xfer has support for I2C_M_RECV_LEN.
+	 * That is OK as msg->len is <= 2 when that flag is set.
 	 */
 	if (msg->len < 8) {
 		ret = mxs_i2c_pio_setup_xfer(adap, msg, flags);

@@ -497,6 +533,8 @@ static irqreturn_t mxs_i2c_isr(int this_irq, void *dev_id)
 	struct mxs_i2c_dev *i2c = dev_id;
 	u32 stat = readl(i2c->regs + MXS_I2C_CTRL1) & MXS_I2C_IRQ_MASK;
 
+	pr_debug("%s: stat = 0x%08x\n", __func__, stat);
+
 	if (!stat)
 		return IRQ_NONE;
 

The problem is that mxs_i2c_pio_wait_dmareq that is called in the if (msg->flags & I2C_M_RECV_LEN) block times out, i.e. the hardware doesn't set MXS_I2C_DEBUG0_DMAREQ after the read request for the length byte. The same problem happens when I use the dma path to implement I2C_M_RECV_LEN support. In that case mxs_i2c_isr is called, but the dma callback is not.

On the wire the following happens for a i2c_smbus_read_block_data call (from userspace):

	S $clientaddr W A $command A Sr $clientaddr R A 0x01

as expected and then the i.MX28 starts sending an ack:

	SDA ..____________/¯¯¯\____
	SCL .._/¯\_/¯\_/¯\_/¯\_/¯¯¯
	    ..  5   6   7   8   9
              slave sending   | mxs starts
	      length byte (1) | to send A

but MXS_I2C_DEBUG0 stays 0 while mxs_i2c_pio_wait_dmareq waits. After that a P is put on the wire. That fits to what I see when using dma:
HW_APBX_CH6_DEBUG1.STATEMACHINE == 0x15 meaning:

WAIT_END — When the Wait for Command End bit is set, the state machine enters this state until the DMA device indicates that the command is complete.

So the problem seems to be that the i2c core fails to tell the dma core that it's done.

Other things I wonder about in the i2c-mxs driver:

- I don't understand why RETAIN_CLOCK is added to MXS_CMD_I2C_SELECT. From my understanding of i2c and the hardware manual I'd say this is only relevant for slave mode? Setting RETAIN_CLOCK for the cycle that reads the length byte is broken in a different way. (After reading the 1 the mxs pulls low SDA for acking but doesn't pulse SCL. Then another 9 clock pulses are sent with SDA high. After the 9th falling edge SDA is pulled low by the i.MX28 and then both lines go back to 1 at the same time. I will debug a bit more here.)
So it seems RETAIN_CLOCK has a meaning for master mode, too. If you have a more concrete perception of its meaning it would be great if you could share it.

- Is it correct to have I2C_FUNC_SMBUS_EMUL in .functionality without supporting I2C_M_RECV_LEN?

Best regards
Uwe

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Hi Uwe,

please prefer wsa-z923LK4zBo2bacvFa/[email protected] for kernel stuff.

Yes. Check the #define and you'll see that it does not contain BLOCK_READ and BLOCK_PROC_CALL.

All the best,

Wolfram

The reason I used wolfram@... is probably:

	$ for addr in sa olfram; do printf "%s: " $addr; git log --oneline --grep=w$addr-z923LK4zBo2bacvFa/[email protected] v3.9-rc6 | wc -l; done
	sa: 7
	olfram: 35

But I see the recent commits use "sa" so probably I will do that next time, too.

Ah, ok. But then there is a different problem: Even though "my" driver only advertises I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL calling i2c_smbus_read_block_data in userspace results in .master_xfer being called with I2C_M_RECV_LEN set.

Best regards
Uwe

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Hi Uwe,

Looking up MAINTAINERS would have done it :)

Because not every I2C or SMBus adapter implements everything in the I2C specifications, a client can not trust that everything it needs is implemented when it is given the option to attach to an adapter:
the client needs some way to check whether an adapter has the needed functionality...

Regards,

Wolfram

Hi Wolfram,

While add support for I2C_M_RECV_LEN I forgot to write the length data to the first byte in the message buffer which happend to be initialized with 0xff. This made i2c_smbus_xfer_emulated copy 255 bytes to data->block overflowing the array and so resulting in stack curruption. I think the same could be accomplished with a non-broken driver (e.g. by calling i2c_smbus_read_block_data for an eeprom that is interpreted as a 1 byte read by the i2c bus driver. If the read byte is big enough the same stack curruption occurs). So IMHO the i2c core should be a bit more careful here and either not let i2c_smbus_xfer_emulated call the xfer callback of a driver that is not capable to handle I2C_M_RECV_LEN with a message that has this bit set or at least assert that data->block isn't written to out of bounds.

Best regards
Uwe

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Hi Uwe,

CCing Lucas Stach too, are you two not in the same office now ? ;-)

Try basing your patch on top of Lucas's, they looked very reasonable. I think WS should have applied them by now. Wolfram, what's the status of those three mxs- i2c patches from Lucas, can you tell?

Best regards,
Marek Vasut