[PATCH] i2c-mpc: Correct I2C reset procedure

[PATCH] i2c-mpc: Correct I2C reset procedure

[Joakim Tjernlund]

Current I2C reset procedure is broken in two ways:
1) It only generate 1 START instead of 9 STARTs and STOP.
2) It leaves the bus Busy so every I2C xfer after the first
   fixup calls the reset routine again, for every xfer there after.

This fixes both errors. Add an iobarrier_rw() when writing the
I2C control register as well to make sure the register reaches the
controller in time.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---

 Not sure where to sent this as there is no maintainer so adding
 Scott Wood as well.

 drivers/i2c/busses/i2c-mpc.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
index 8393140..09b826d 100644
--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -86,6 +86,7 @@ struct mpc_i2c_data {
 static inline void writeccr(struct mpc_i2c *i2c, u32 x)
 {
 	writeb(x, i2c->base + MPC_I2C_CR);
+	iobarrier_rw();
 }
 
 static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
@@ -104,23 +105,30 @@ static irqreturn_t mpc_i2c_isr(int irq, void *dev_id)
 /* Sometimes 9th clock pulse isn't generated, and slave doesn't release
  * the bus, because it wants to send ACK.
  * Following sequence of enabling/disabling and sending start/stop generates
- * the 9 pulses, so it's all OK.
+ * the 9 pulses, each with a START then ending with STOP, so it's all OK.
  */
 static void mpc_i2c_fixup(struct mpc_i2c *i2c)
 {
 	int k;
-	u32 delay_val = 1000000 / i2c->real_clk + 1;
-
-	if (delay_val < 2)
-		delay_val = 2;
+	unsigned long flags;
 
 	for (k = 9; k; k--) {
 		writeccr(i2c, 0);
-		writeccr(i2c, CCR_MSTA | CCR_MTX | CCR_MEN);
+		writeb(0, i2c->base + MPC_I2C_SR); /* clear any status bits */
+		writeccr(i2c, CCR_MEN | CCR_MSTA); /* START */
+		readb(i2c->base + MPC_I2C_DR); /* init xfer */
+		udelay(15); /* let it hit the bus */
+		local_irq_save(flags); /* should not be delayed further */
+		writeccr(i2c, CCR_MEN | CCR_MSTA | CCR_RSTA); /* delay SDA */
 		readb(i2c->base + MPC_I2C_DR);
-		writeccr(i2c, CCR_MEN);
-		udelay(delay_val << 1);
+		if (k != 1)
+			udelay(5);
+		local_irq_restore(flags);
 	}
+	writeccr(i2c, CCR_MEN); /* Initiate STOP */
+	readb(i2c->base + MPC_I2C_DR);
+	udelay(15); /* Let STOP propagate */
+	writeccr(i2c, 0);
 }
 
 static int i2c_wait(struct mpc_i2c *i2c, unsigned timeout, int writing)
-- 
2.10.2

Re: i2c-mpc: Correct I2C reset procedure

[Scott Wood]

On Tue, May 09, 2017 at 02:03:51PM +0200, Joakim Tjernlund wrote:
> Current I2C reset procedure is broken in two ways:
> 1) It only generate 1 START instead of 9 STARTs and STOP.
> 2) It leaves the bus Busy so every I2C xfer after the first
>    fixup calls the reset routine again, for every xfer there after.
> 
> This fixes both errors. Add an iobarrier_rw() when writing the
> I2C control register as well to make sure the register reaches the
> controller in time.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> ---
> 
>  Not sure where to sent this as there is no maintainer so adding
>  Scott Wood as well.
> 
>  drivers/i2c/busses/i2c-mpc.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> index 8393140..09b826d 100644
> --- a/drivers/i2c/busses/i2c-mpc.c
> +++ b/drivers/i2c/busses/i2c-mpc.c
> @@ -86,6 +86,7 @@ struct mpc_i2c_data {
>  static inline void writeccr(struct mpc_i2c *i2c, u32 x)
>  {
>  	writeb(x, i2c->base + MPC_I2C_CR);
> +	iobarrier_rw();
>  }

Why are the barriers in the I/O accessors insufficient?

-Scott

Re: i2c-mpc: Correct I2C reset procedure

[Joakim Tjernlund]

On Tue, 2017-05-09 at 15:54 -0500, Scott Wood wrote:
> On Tue, May 09, 2017 at 02:03:51PM +0200, Joakim Tjernlund wrote:
> > Current I2C reset procedure is broken in two ways:
> > 1) It only generate 1 START instead of 9 STARTs and STOP.
> > 2) It leaves the bus Busy so every I2C xfer after the first
> >    fixup calls the reset routine again, for every xfer there after.
> > 
> > This fixes both errors. Add an iobarrier_rw() when writing the
> > I2C control register as well to make sure the register reaches the
> > controller in time.
> > 
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > ---
> > 
> >  Not sure where to sent this as there is no maintainer so adding
> >  Scott Wood as well.
> > 
> >  drivers/i2c/busses/i2c-mpc.c | 24 ++++++++++++++++--------
> >  1 file changed, 16 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > index 8393140..09b826d 100644
> > --- a/drivers/i2c/busses/i2c-mpc.c
> > +++ b/drivers/i2c/busses/i2c-mpc.c
> > @@ -86,6 +86,7 @@ struct mpc_i2c_data {
> >  static inline void writeccr(struct mpc_i2c *i2c, u32 x)
> >  {
> >  	writeb(x, i2c->base + MPC_I2C_CR);
> > +	iobarrier_rw();
> >  }
> 
> Why are the barriers in the I/O accessors insufficient?

You mean writeb()?  As far as I can see the writeb/readb only uses volatile and that
can be a bit weak for ppc, even on guarded, uncached memory mappings.
I wanted to make sure multiple writeb did hit the controller correctly.

 Jocke 

Re: i2c-mpc: Correct I2C reset procedure

[Scott Wood]

On Wed, 2017-05-10 at 07:02 +0000, Joakim Tjernlund wrote:
> On Tue, 2017-05-09 at 15:54 -0500, Scott Wood wrote:
> > On Tue, May 09, 2017 at 02:03:51PM +0200, Joakim Tjernlund wrote:
> > > Current I2C reset procedure is broken in two ways:
> > > 1) It only generate 1 START instead of 9 STARTs and STOP.
> > > 2) It leaves the bus Busy so every I2C xfer after the first
> > >    fixup calls the reset routine again, for every xfer there after.
> > > 
> > > This fixes both errors. Add an iobarrier_rw() when writing the
> > > I2C control register as well to make sure the register reaches the
> > > controller in time.
> > > 
> > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > ---
> > > 
> > >  Not sure where to sent this as there is no maintainer so adding
> > >  Scott Wood as well.
> > > 
> > >  drivers/i2c/busses/i2c-mpc.c | 24 ++++++++++++++++--------
> > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > > index 8393140..09b826d 100644
> > > --- a/drivers/i2c/busses/i2c-mpc.c
> > > +++ b/drivers/i2c/busses/i2c-mpc.c
> > > @@ -86,6 +86,7 @@ struct mpc_i2c_data {
> > >  static inline void writeccr(struct mpc_i2c *i2c, u32 x)
> > >  {
> > >  	writeb(x, i2c->base + MPC_I2C_CR);
> > > +	iobarrier_rw();
> > >  }
> > 
> > Why are the barriers in the I/O accessors insufficient?
> 
> You mean writeb()?  As far as I can see the writeb/readb only uses volatile
> and that
> can be a bit weak for ppc, even on guarded, uncached memory mappings.
> I wanted to make sure multiple writeb did hit the controller correctly.

It's not just a volatile.  There's a sync before each access, and a twi/isync
after loads.  writeb() maps to __do_writeb() which maps to out_8() which is
implemented with DEF_MMIO_OUT_D.

-Scott

Re: i2c-mpc: Correct I2C reset procedure

[Joakim Tjernlund]

On Wed, 2017-05-10 at 20:42 -0500, Scott Wood wrote:
> On Wed, 2017-05-10 at 07:02 +0000, Joakim Tjernlund wrote:
> > On Tue, 2017-05-09 at 15:54 -0500, Scott Wood wrote:
> > > On Tue, May 09, 2017 at 02:03:51PM +0200, Joakim Tjernlund wrote:
> > > > Current I2C reset procedure is broken in two ways:
> > > > 1) It only generate 1 START instead of 9 STARTs and STOP.
> > > > 2) It leaves the bus Busy so every I2C xfer after the first
> > > >    fixup calls the reset routine again, for every xfer there after.
> > > > 
> > > > This fixes both errors. Add an iobarrier_rw() when writing the
> > > > I2C control register as well to make sure the register reaches the
> > > > controller in time.
> > > > 
> > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> > > > ---
> > > > 
> > > >  Not sure where to sent this as there is no maintainer so adding
> > > >  Scott Wood as well.
> > > > 
> > > >  drivers/i2c/busses/i2c-mpc.c | 24 ++++++++++++++++--------
> > > >  1 file changed, 16 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c
> > > > index 8393140..09b826d 100644
> > > > --- a/drivers/i2c/busses/i2c-mpc.c
> > > > +++ b/drivers/i2c/busses/i2c-mpc.c
> > > > @@ -86,6 +86,7 @@ struct mpc_i2c_data {
> > > >  static inline void writeccr(struct mpc_i2c *i2c, u32 x)
> > > >  {
> > > >  	writeb(x, i2c->base + MPC_I2C_CR);
> > > > +	iobarrier_rw();
> > > >  }
> > > 
> > > Why are the barriers in the I/O accessors insufficient?
> > 
> > You mean writeb()?  As far as I can see the writeb/readb only uses volatile
> > and that
> > can be a bit weak for ppc, even on guarded, uncached memory mappings.
> > I wanted to make sure multiple writeb did hit the controller correctly.
> 
> It's not just a volatile.  There's a sync before each access, and a twi/isync
> after loads.  writeb() maps to __do_writeb() which maps to out_8() which is
> implemented with DEF_MMIO_OUT_D.

ehh, right you are. That was some very complicated include/#ifdef trickery to go through.
I will send an update patch with the iobarrier_rw() removed.

 Jocke

Re: [PATCH] i2c-mpc: Correct I2C reset procedure

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 637 bytes --]

On Tue, May 09, 2017 at 02:03:51PM +0200, Joakim Tjernlund wrote:
> Current I2C reset procedure is broken in two ways:
> 1) It only generate 1 START instead of 9 STARTs and STOP.
> 2) It leaves the bus Busy so every I2C xfer after the first
>    fixup calls the reset routine again, for every xfer there after.
> 
> This fixes both errors. Add an iobarrier_rw() when writing the
> I2C control register as well to make sure the register reaches the
> controller in time.
> 
> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>

BTW can this driver be converted to make use of the bus_recovery
infrastructure?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c-mpc: Correct I2C reset procedure

[Joakim Tjernlund]

On Thu, 2017-05-11 at 13:28 +0200, Wolfram Sang wrote:
> On Tue, May 09, 2017 at 02:03:51PM +0200, Joakim Tjernlund wrote:
> > Current I2C reset procedure is broken in two ways:
> > 1) It only generate 1 START instead of 9 STARTs and STOP.
> > 2) It leaves the bus Busy so every I2C xfer after the first
> >    fixup calls the reset routine again, for every xfer there after.
> > 
> > This fixes both errors. Add an iobarrier_rw() when writing the
> > I2C control register as well to make sure the register reaches the
> > controller in time.
> > 
> > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
> 
> BTW can this driver be converted to make use of the bus_recovery
> infrastructure?
> 

Maybe, I not familiar with this infrastructure. I did take a look at i2c_generic_recovery()
though and I do not agree with that impl.

The 9 clk's fixup does not cover the the case when an I2C device is stuck write.
To fix stuck in both cases(read or write) you need to generate 9 clk's with a
START in each clk, then a STOP to release the bus.

 Jocke