| Message ID | 20170509120351.28273-1-joakim.tjernlund@infinera.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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?
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
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)
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(-)