Patchwork i2c-mpc: generate START condition after STOP caused by read i2c_msg

login
register
mail settings
Submitter Joakim Tjernlund
Date May 14, 2009, 4:52 p.m.
Message ID <OF3CD158BA.04CCFBB5-ONC12575B6.005911D5-C12575B6.005CAFC2@transmode.se>
Download mbox | patch
Permalink /patch/27216/
State Superseded
Delegated to: Kumar Gala
Headers show

Comments

Joakim Tjernlund - May 14, 2009, 4:52 p.m.
Esben Haabendal <esbenhaabendal@gmail.com> wrote on 14/05/2009 12:17:48:
> On Thu, May 14, 2009 at 11:58 AM, Joakim Tjernlund
> <joakim.tjernlund@transmode.se> wrote:
> >>
> >> The problem is that after the STOP condition, the following i2c_msg will be
> >> attempted with a repeated START, which according to specification will
> >> cause a MAL, which also happens.  My MPC8360ERM reads:
> >>
> >> "Attempting a repeated START at the wrong time (or if the bus is owned
> >> by another master), results in loss of arbitration."
> >>
> >> Which I know read as it that we must own the I2C bus when using RSTA.
> >
> > I agree with the theory, but isn't the problem here that STOP is performed in
> > the middle of this transaction?
> > Remove the STOP and RSTA will work I think.
>
> That was also my first idea.
>
> But at least for my CS4265, reads does not work without a STOP following the
> last byte. I guess I am not the first to experience this, as indicated
> by the current
> i2c-mpc.c implementation.
> As far as I can understand the I2C specification, a STOP should not be required
> after reads, but I am not sure.
>
> The cost of the additional STOP is really small.  The only issue I can see,
> is that with the STOP in the midle of an i2c_transfer, the I2C bus is released.
> This naturally also happens with the current implementation, so I believe
> that this patch is a clean improvement.
>
> Anyways, if someone can figure out how to achive this without STOP,
> I will be happy (re)test with my hardware setup here.

From just playing a little, I think the below patch will get you far.
However, I had to change this too to make it stable:
static void mpc_i2c_start(struct mpc_i2c *i2c)
{
	/* Clear arbitration */
	writeb(0, i2c->base + MPC_I2C_SR);
#if 1 // fix, not sure why
	writeccr(i2c, 0);
	udelay(5);
#endif
	/* Start with MEN */
	writeccr(i2c, CCR_MEN);
}

PS.
   I added back the linuxppc list, let it stay.
Esben Haabendal - May 15, 2009, 10:18 a.m.
Hi

Your patch (and the small addition to mpc_i2c_start) does not work for me.

[   35.765803] mpc_xfer()
[   35.785480] writeccr 0
[   35.785505] writeccr 80
[   35.785523] mpc_xfer: 1 bytes to 2c:W - 1 of 2 messages
[   35.798817] mpc_write addr=2c len=1 restart=0
[   35.815327] writeccr f0
[   35.815503] I2C: SR=a2
[   35.818675] I2C: SR=a6
[   35.821450] mpc_xfer: 1 bytes to 2c:R - 2 of 2 messages
[   35.827119] mpc_read addr=2c len=1 restart=1
[   35.837463] writeccr f4
[   35.837641] I2C: SR=a6
[   35.840011] writeccr e8
[   35.840133] I2C: SR=a3
[   35.843596] writeccr 80
[   35.843632] mpc_xfer()
[   35.855068] writeccr 0
[   35.855093] writeccr 80
[   35.855111] mpc_xfer: 1 bytes to 2c:W - 1 of 2 messages
[   35.865346] mpc_write addr=2c len=1 restart=0
[   35.870109] writeccr f0
[   35.870272] I2C: SR=a2
[   35.873372] I2C: SR=a6
[   35.875757] mpc_xfer: 1 bytes to 2c:R - 2 of 2 messages
[   35.881606] mpc_read addr=2c len=1 restart=1
[   35.886290] writeccr f4
[   35.886463] I2C: SR=a6
[   35.889425] writeccr e8
[   35.889575] I2C: SR=a7
[   35.891944] writeccr 80
[   35.961177] mpc_xfer()
[   35.972517] writeccr 0
[   35.972541] writeccr 80
[   35.972559] mpc_xfer: 1 bytes to 4e:W - 1 of 20 messages
[   35.982628] mpc_write addr=4e len=1 restart=0
[   35.987389] writeccr f0
[   35.987424] I2C: SR=b3
[   35.990386] I2C: MAL
[   35.992971] i2c_wait(address) error: -5
[   35.997215] writeccr 80
[   35.997241] Error: i2c_transfer failed: -5

I have now spent a few hours trying a lot of different paths to fix
this approach, but I simply cannot find a way to get i2c read to work
without a trailing STOP condition with this controller.

Is there a problem in applying my patch, as it should be "clean"
improvement over the current implementation, which uses STOP
condition, but does not work.

Until Isomeone finds a way to get it to work without STOP, we will
have a working driver, which I assume is what we want.

Best regards,
Esben Haabendal

Patch

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 6c1cddd..d7edcd2 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -189,9 +189,6 @@  static int mpc_write(struct mpc_i2c *i2c, int target,
 	unsigned timeout = i2c->adap.timeout;
 	u32 flags = restart ? CCR_RSTA : 0;

-	/* Start with MEN */
-	if (!restart)
-		writeccr(i2c, CCR_MEN);
 	/* Start as master */
 	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX | flags);
 	/* Write target byte */
@@ -220,9 +217,6 @@  static int mpc_read(struct mpc_i2c *i2c, int target,
 	int i, result;
 	u32 flags = restart ? CCR_RSTA : 0;

-	/* Start with MEN */
-	if (!restart)
-		writeccr(i2c, CCR_MEN);
 	/* Switch to read - restart */
 	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX | flags);
 	/* Write target address byte - this time with the read flag set */
@@ -241,18 +235,15 @@  static int mpc_read(struct mpc_i2c *i2c, int target,
 		readb(i2c->base + MPC_I2C_DR);
 	}

-	for (i = 0; i < length; i++) {
+	for (i = length; i ; --i) {
 		result = i2c_wait(i2c, timeout, 0);
 		if (result < 0)
 			return result;

-		/* Generate txack on next to last byte */
-		if (i == length - 2)
+		/* Generate txack on next to last byte(s) */
+		if (i <= 2)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
-		/* Generate stop on last byte */
-		if (i == length - 1)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
-		data[i] = readb(i2c->base + MPC_I2C_DR);
+		data[length-i] = readb(i2c->base + MPC_I2C_DR);
 	}

 	return length;