I2C_M_RECV_LEN for i2c-mxs

I2C_M_RECV_LEN for i2c-mxs

[Uwe Kleine-König]

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:

diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -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/  |

Re: I2C_M_RECV_LEN for i2c-mxs

[Wolfram Sang]

Hi Uwe,

please prefer wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org for kernel stuff.

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

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

All the best,

   Wolfram

Re: I2C_M_RECV_LEN for i2c-mxs

[Uwe Kleine-König]

On Fri, Apr 12, 2013 at 05:37:57PM +0200, Wolfram Sang wrote:
> please prefer wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org for kernel stuff.
The reason I used wolfram@... is probably:

	$ for addr in sa olfram; do printf "%s: " $addr; git log --oneline --grep=w$addr-z923LK4zBo2bacvFa/9K2g@public.gmane.org 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.

> >  - Is it correct to have I2C_FUNC_SMBUS_EMUL in .functionality without
> >    supporting I2C_M_RECV_LEN?
> 
> Yes. Check the #define and you'll see that it does not contain
> BLOCK_READ and BLOCK_PROC_CALL.
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/  |

Re: I2C_M_RECV_LEN for i2c-mxs

[Wolfram Sang]

Hi Uwe,

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

Looking up MAINTAINERS would have done it :)

> 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.

>From Documentation/i2c/functionality:

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

Re: I2C_M_RECV_LEN for i2c-mxs

[Marek Vasut]

Hi Uwe,

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

> 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:

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

Re: I2C_M_RECV_LEN for i2c-mxs

[Uwe Kleine-König]

Hi Wolfram,

On Sun, Apr 14, 2013 at 01:57:58PM +0200, Wolfram Sang wrote:
> > 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.
> 
> From Documentation/i2c/functionality:
> 
> 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...
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/  |