[PATCH] i2c-mpc: Do not generate STOP after read.

[PATCH] i2c-mpc: Do not generate STOP after read.

[Joakim Tjernlund]

The driver always ends a read with a STOP condition which
breaks subsequent I2C reads/writes in the same transaction as
these expect to do a repeated START(ReSTART).

This will also help I2C multimaster as the bus will not be released
after the first read, but when the whole transaction ends.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
---

This should also fix a problem reported by Esben Haabendal:

[PATCH v2] i2c-mpc: generate START condition after STOP caused by read i2c_msg

This fixes MAL (arbitration lost) bug caused by illegal use of
RSTA (repeated START) after STOP condition generated after last byte
of reads. With this patch, it is possible to do an i2c_transfer() with
additional i2c_msg's following the I2C_M_RD messages.

It still needs to be resolved if it is possible to fix this issue
by removing the STOP condition after reads in a robust way.

Signed-off-by: Esben Haabendal <eha-SIcX2qgNSybd/EuGEV170n9LOBIZ5rWg@public.gmane.org>

 drivers/i2c/busses/i2c-mpc.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index cdb1858..88ae582 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -369,9 +369,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 */
@@ -400,9 +397,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 */
@@ -429,9 +423,9 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
 		/* Generate txack on next to last byte */
 		if (i == length - 2)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
-		/* Generate stop on last byte */
+		/* Do not generate stop on last byte */
 		if (i == length - 1)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
+			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
 		data[i] = readb(i2c->base + MPC_I2C_DR);
 	}
 
-- 
1.6.4.3

[PATCH] i2c-mpc: Do not generate STOP after read.

[Joakim Tjernlund]

The driver always ends a read with a STOP condition which
breaks subsequent I2C reads/writes in the same transaction as
these expect to do a repeated START(ReSTART).

This will also help I2C multimaster as the bus will not be released
after the first read, but when the whole transaction ends.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---

This should also fix a problem reported by Esben Haabendal:

[PATCH v2] i2c-mpc: generate START condition after STOP caused by read i2c_msg

This fixes MAL (arbitration lost) bug caused by illegal use of
RSTA (repeated START) after STOP condition generated after last byte
of reads. With this patch, it is possible to do an i2c_transfer() with
additional i2c_msg's following the I2C_M_RD messages.

It still needs to be resolved if it is possible to fix this issue
by removing the STOP condition after reads in a robust way.

Signed-off-by: Esben Haabendal <eha@doredevelopment.dk>

 drivers/i2c/busses/i2c-mpc.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index cdb1858..88ae582 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -369,9 +369,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 */
@@ -400,9 +397,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 */
@@ -429,9 +423,9 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
 		/* Generate txack on next to last byte */
 		if (i == length - 2)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
-		/* Generate stop on last byte */
+		/* Do not generate stop on last byte */
 		if (i == length - 1)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
+			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
 		data[i] = readb(i2c->base + MPC_I2C_DR);
 	}
 
-- 
1.6.4.3

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Wolfgang Grandegger]

Joakim Tjernlund wrote:
> The driver always ends a read with a STOP condition which
> breaks subsequent I2C reads/writes in the same transaction as
> these expect to do a repeated START(ReSTART).
> 
> This will also help I2C multimaster as the bus will not be released
> after the first read, but when the whole transaction ends.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
Tested-by: Wolfgang Grandegger <wg-5Yr1BZd7O62+XT7JhA+gdA@public.gmane.org>

on a MPC8548 board with an up-to-date kernel. I did not realize any
problems.

Wolfgang.

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Wolfgang Grandegger]

Joakim Tjernlund wrote:
> The driver always ends a read with a STOP condition which
> breaks subsequent I2C reads/writes in the same transaction as
> these expect to do a repeated START(ReSTART).
> 
> This will also help I2C multimaster as the bus will not be released
> after the first read, but when the whole transaction ends.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Tested-by: Wolfgang Grandegger <wg@grandegger.com>

on a MPC8548 board with an up-to-date kernel. I did not realize any
problems.

Wolfgang.

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Joakim Tjernlund]

Jean, I just noticed you pull request for i2c on LKML but I didn't see this
patch nor have I got any feedback from you. What is your view?

   Jocke

Wolfgang Grandegger <wg@grandegger.com> wrote on 25/09/2009 12:01:17:
>
> Joakim Tjernlund wrote:
> > The driver always ends a read with a STOP condition which
> > breaks subsequent I2C reads/writes in the same transaction as
> > these expect to do a repeated START(ReSTART).
> >
> > This will also help I2C multimaster as the bus will not be released
> > after the first read, but when the whole transaction ends.
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
> Tested-by: Wolfgang Grandegger <wg@grandegger.com>
>
> on a MPC8548 board with an up-to-date kernel. I did not realize any
> problems.
>
> Wolfgang.
>

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Jean Delvare]

On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote:
> Jean, I just noticed you pull request for i2c on LKML but I didn't see this
> patch nor have I got any feedback from you. What is your view?

My view is that i2c-mpc is nor under my jurisdiction, so I did not, and
will not, pay any attention to it.

-- 
Jean Delvare

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Jean Delvare]

On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote:
> Jean, I just noticed you pull request for i2c on LKML but I didn't see this
> patch nor have I got any feedback from you. What is your view?

My view is that i2c-mpc is nor under my jurisdiction, so I did not, and
will not, pay any attention to it.

-- 
Jean Delvare

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Joakim Tjernlund]

Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote on 28/09/2009 09:28:09:
>
> On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote:
> > Jean, I just noticed you pull request for i2c on LKML but I didn't see this
> > patch nor have I got any feedback from you. What is your view?
>
> My view is that i2c-mpc is nor under my jurisdiction, so I did not, and
> will not, pay any attention to it.

Ah, that explains it. Who then will look after i2c-mpc? Kumar?

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Joakim Tjernlund]

Jean Delvare <khali@linux-fr.org> wrote on 28/09/2009 09:28:09:
>
> On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote:
> > Jean, I just noticed you pull request for i2c on LKML but I didn't see this
> > patch nor have I got any feedback from you. What is your view?
>
> My view is that i2c-mpc is nor under my jurisdiction, so I did not, and
> will not, pay any attention to it.

Ah, that explains it. Who then will look after i2c-mpc? Kumar?

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Peter Korsgaard]

>>>>> "Joakim" == Joakim Tjernlund <joakim.tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org> writes:

Hi,

 Joakim> Ah, that explains it. Who then will look after i2c-mpc? Kumar?

Ben Dooks (embedded i2c maintainer). He's afaik coming home today, so
give him a few days to catch up on mails.

-- 
Bye, Peter Korsgaard

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Peter Korsgaard]

>>>>> "Joakim" == Joakim Tjernlund <joakim.tjernlund@transmode.se> writes:

Hi,

 Joakim> Ah, that explains it. Who then will look after i2c-mpc? Kumar?

Ben Dooks (embedded i2c maintainer). He's afaik coming home today, so
give him a few days to catch up on mails.

-- 
Bye, Peter Korsgaard

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Jean Delvare]

On Mon, 28 Sep 2009 09:30:32 +0200, Joakim Tjernlund wrote:
> Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote on 28/09/2009 09:28:09:
> >
> > On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote:
> > > Jean, I just noticed you pull request for i2c on LKML but I didn't see this
> > > patch nor have I got any feedback from you. What is your view?
> >
> > My view is that i2c-mpc is nor under my jurisdiction, so I did not, and
> > will not, pay any attention to it.
> 
> Ah, that explains it. Who then will look after i2c-mpc? Kumar?

What does MAINTAINERS say?

-- 
Jean Delvare

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Jean Delvare]

On Mon, 28 Sep 2009 09:30:32 +0200, Joakim Tjernlund wrote:
> Jean Delvare <khali@linux-fr.org> wrote on 28/09/2009 09:28:09:
> >
> > On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote:
> > > Jean, I just noticed you pull request for i2c on LKML but I didn't see this
> > > patch nor have I got any feedback from you. What is your view?
> >
> > My view is that i2c-mpc is nor under my jurisdiction, so I did not, and
> > will not, pay any attention to it.
> 
> Ah, that explains it. Who then will look after i2c-mpc? Kumar?

What does MAINTAINERS say?

-- 
Jean Delvare

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Joakim Tjernlund]

Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote on 28/09/2009 09:34:34:
>
> On Mon, 28 Sep 2009 09:30:32 +0200, Joakim Tjernlund wrote:
> > Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote on 28/09/2009 09:28:09:
> > >
> > > On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote:
> > > > Jean, I just noticed you pull request for i2c on LKML but I didn't see this
> > > > patch nor have I got any feedback from you. What is your view?
> > >
> > > My view is that i2c-mpc is nor under my jurisdiction, so I did not, and
> > > will not, pay any attention to it.
> >
> > Ah, that explains it. Who then will look after i2c-mpc? Kumar?
>
> What does MAINTAINERS say?

I did a grep for i2c-mpc and nothing turned up. Now that someone mentioned
Ben Dooks I found it.

Re: [PATCH] i2c-mpc: Do not generate STOP after read.

[Joakim Tjernlund]

Jean Delvare <khali@linux-fr.org> wrote on 28/09/2009 09:34:34:
>
> On Mon, 28 Sep 2009 09:30:32 +0200, Joakim Tjernlund wrote:
> > Jean Delvare <khali@linux-fr.org> wrote on 28/09/2009 09:28:09:
> > >
> > > On Mon, 28 Sep 2009 00:26:54 +0200, Joakim Tjernlund wrote:
> > > > Jean, I just noticed you pull request for i2c on LKML but I didn't see this
> > > > patch nor have I got any feedback from you. What is your view?
> > >
> > > My view is that i2c-mpc is nor under my jurisdiction, so I did not, and
> > > will not, pay any attention to it.
> >
> > Ah, that explains it. Who then will look after i2c-mpc? Kumar?
>
> What does MAINTAINERS say?

I did a grep for i2c-mpc and nothing turned up. Now that someone mentioned
Ben Dooks I found it.

[PATCH] i2c-mpc: Do not generate STOP after read.

[Joakim Tjernlund]

The driver always ends a read with a STOP condition which
breaks subsequent I2C reads/writes in the same transaction as
these expect to do a repeated START(ReSTART).

This will also help I2C multimaster as the bus will not be released
after the first read, but when the whole transaction ends.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
---

Andrew,

Seems like nobody wants to touch this patch. Jean Delvare
refers to Ben Dooks and he has not replied to any of my mails.
Perhaps you can carry it in you patch queue?

 drivers/i2c/busses/i2c-mpc.c |   10 ++--------
 1 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index cdb1858..88ae582 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -369,9 +369,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 */
@@ -400,9 +397,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 */
@@ -429,9 +423,9 @@ static int mpc_read(struct mpc_i2c *i2c, int target,
 		/* Generate txack on next to last byte */
 		if (i == length - 2)
 			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_TXAK);
-		/* Generate stop on last byte */
+		/* Do not generate stop on last byte */
 		if (i == length - 1)
-			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK);
+			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MSTA | CCR_MTX);
 		data[i] = readb(i2c->base + MPC_I2C_DR);
 	}
 
-- 
1.6.4.3