i2c-mpc: Correct I2C reset procedure

Message ID 20170509120351.28273-1-joakim.tjernlund@infinera.com
State New
Headers show

Commit Message

Joakim Tjernlund May 9, 2017, 12:03 p.m.
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(-)

Comments

Scott Wood May 9, 2017, 8:54 p.m. | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joakim Tjernlund May 10, 2017, 7:02 a.m. | #2
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 --
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood May 11, 2017, 1:42 a.m. | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joakim Tjernlund May 11, 2017, 11:23 a.m. | #4
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--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang May 11, 2017, 11:28 a.m. | #5
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?
Joakim Tjernlund May 11, 2017, 11:42 a.m. | #6
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  --
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)