| Message ID | 20170511122033.22471-1-joakim.tjernlund@infinera.com |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, 2017-05-11 at 14:20 +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. Ping? > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> > --- > > v2 - Remove io barrier call. > drivers/i2c/busses/i2c-mpc.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c > index 8393140..6b5e6ce4 100644 > --- a/drivers/i2c/busses/i2c-mpc.c > +++ b/drivers/i2c/busses/i2c-mpc.c > @@ -104,23 +104,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) -- 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-16 at 17:13 +0200, Joakim Tjernlund wrote: > On Thu, 2017-05-11 at 14:20 +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. > > Ping? Ping again, pretty please? > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> > > --- > > > > v2 - Remove io barrier call. > > drivers/i2c/busses/i2c-mpc.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c > > index 8393140..6b5e6ce4 100644 > > --- a/drivers/i2c/busses/i2c-mpc.c > > +++ b/drivers/i2c/busses/i2c-mpc.c > > @@ -104,23 +104,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) -- 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-23 at 13:47 +0000, Joakim Tjernlund wrote: > On Tue, 2017-05-16 at 17:13 +0200, Joakim Tjernlund wrote: > > On Thu, 2017-05-11 at 14:20 +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. > > > > Ping? > > Ping again, pretty please? Acked-by: Scott Wood <oss@buserror.net> -Scott > > > > > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> > > > --- > > > > > > v2 - Remove io barrier call. > > > drivers/i2c/busses/i2c-mpc.c | 23 +++++++++++++++-------- > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c > > > index 8393140..6b5e6ce4 100644 > > > --- a/drivers/i2c/busses/i2c-mpc.c > > > +++ b/drivers/i2c/busses/i2c-mpc.c > > > @@ -104,23 +104,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) > > -- 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 23, 2017 at 01:47:34PM +0000, Joakim Tjernlund wrote: > On Tue, 2017-05-16 at 17:13 +0200, Joakim Tjernlund wrote: > > On Thu, 2017-05-11 at 14:20 +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. > > > > Ping? > > Ping again, pretty please? I am interested in this patch. Especially why you think the generic recovery structure is not suitable and if we maybe need to update that, too. But I need time to think into it and that may still need 1-2 weeks. I am aiming for 4.12 nonetheless.
On Mon, 2017-05-29 at 23:04 +0200, Wolfram Sang wrote: > On Tue, May 23, 2017 at 01:47:34PM +0000, Joakim Tjernlund wrote: > > On Tue, 2017-05-16 at 17:13 +0200, Joakim Tjernlund wrote: > > > On Thu, 2017-05-11 at 14:20 +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. > > > > > > Ping? > > > > Ping again, pretty please? > > I am interested in this patch. Especially why you think the generic > recovery structure is not suitable and if we maybe need to update that, I did my I2C reset analysis many years ago but I found the reason in som old code I had lying around, just sending 9 clocks will not recover the case when stuck in an I2C write sequence. Adding START into the sequence keeps the device in START once it has seen at least one START, then the STOP will release the bus in the end. Other that that I am just not familiar enough with the new recovery structure. > too. But I need time to think into it and that may still need 1-2 weeks. > I am aiming for 4.12 nonetheless. Thanks, I will probably loose track of this one if it doesn't. 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 Mon, 2017-05-29 at 23:04 +0200, Wolfram Sang wrote: > On Tue, May 23, 2017 at 01:47:34PM +0000, Joakim Tjernlund wrote: > > On Tue, 2017-05-16 at 17:13 +0200, Joakim Tjernlund wrote: > > > On Thu, 2017-05-11 at 14:20 +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. > > > > > > Ping? > > > > Ping again, pretty please? > > I am interested in this patch. Especially why you think the generic > recovery structure is not suitable and if we maybe need to update that, > too. But I need time to think into it and that may still need 1-2 weeks. > I am aiming for 4.12 nonetheless. > I don't see this anywhere ... -- 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
> I did my I2C reset analysis many years ago but I found the reason in > som old code I had lying around, just sending 9 clocks will not > recover the case when stuck in an I2C write sequence. Adding START > into the sequence keeps the device in START once it has seen at least > one START, then the STOP will release the bus in the end. OK. I had some time to dig into the specs and understand why I was a bit cautious: Chapter 3.1.16 of the I2C specs describes the procedure of 9x toggling to be done when SDA is stuck low (which is super bad because you can't signal a STOP then). SDA can only stuck low in the read case. In the write case, the master controls SDA, and thus, you should be able to signal STOP immediately. So, for the write case, I'd think a single STOP before toggling should do. Toggling 9x even means you could then write something somewhere which in case of a PMIC can be really dangerous. (BTW I just noticed that the generic recovery routines in the I2C core do not send any STOP. Neither before nor after the toggling. This might need fixing but is a seperate issue). This is all from a theoretical point of view. I don't have a test case for that. Do you have a reproducibly misbehaving device?
> Toggling 9x even means you could then write something somewhere > which in case of a PMIC can be really dangerous. I am partly wrong here because you send a START beforehand. And devices are required to reset their state machine when they detect a START (I2C Specs 3.1.10, Note 4). So, it *shouldn't* be dangerous. If all devices follow that rule, that is... However, you can only send START when SDA is not stuck. And still, this whole toggling is to reanimate a stuck SDA. So, it still looks to me that it doesn't make sense to have START & STOP around the toggling and rather have a single STOP before you try toggling. Makes sense?
On Wed, 2017-06-21 at 23:59 +0200, Wolfram Sang wrote: > > Toggling 9x even means you could then write something somewhere > > which in case of a PMIC can be really dangerous. > > I am partly wrong here because you send a START beforehand. And devices > are required to reset their state machine when they detect a START (I2C > Specs 3.1.10, Note 4). So, it *shouldn't* be dangerous. If all devices > follow that rule, that is... > > However, you can only send START when SDA is not stuck. And still, this > whole toggling is to reanimate a stuck SDA. So, it still looks to me > that it doesn't make sense to have START & STOP around the toggling and > rather have a single STOP before you try toggling. > > Makes sense? STOP must be last so the bus is released, this was one of the problems the patch fixed as next transfer would find the bus busy and the went into fixup again. In general you cannot known if the slave stuck in somewhere in READ/WRITE or some other state. Stuck is probably the wrong word here, not in sync is a better description. If you are afraid the slave won't see the the STARTs why would it see any STOPs? Take this case: > So, for the write case, I'd think a single STOP before toggling should > do. Toggling 9x even means you could then write something somewhere > which in case of a PMIC can be really dangerous. Why would a STOP do anything here? The device is not listening until you get to end of the byte where it will listen to NACK, STOP or START. The best way to get the slaves attention is the send the 9 clocks with a START, if possible, in each clock. That will get the slaves attention and place the slave in START state, then you finish with a STOP so all devices, including any masters, sees that the bus is free. Jocke
On Thu, Jun 22, 2017 at 08:40:25AM +0000, Joakim Tjernlund wrote: > On Wed, 2017-06-21 at 23:59 +0200, Wolfram Sang wrote: > > > Toggling 9x even means you could then write something somewhere > > > which in case of a PMIC can be really dangerous. > > > > I am partly wrong here because you send a START beforehand. And devices > > are required to reset their state machine when they detect a START (I2C > > Specs 3.1.10, Note 4). So, it *shouldn't* be dangerous. If all devices > > follow that rule, that is... > > > > However, you can only send START when SDA is not stuck. And still, this > > whole toggling is to reanimate a stuck SDA. So, it still looks to me > > that it doesn't make sense to have START & STOP around the toggling and > > rather have a single STOP before you try toggling. > > > > Makes sense? > > STOP must be last so the bus is released, this was one of the problems > the patch fixed as next transfer would find the bus busy and the went > into fixup again. Yes, agreed. STOP must be last. > In general you cannot known if the slave stuck in somewhere in > READ/WRITE or some other state. Stuck is probably the wrong word here, > not in sync is a better description. I think those are two different issues. The bus can be stuck, namely when SCL or SDA is stuck low. We can't do anything about SCL other than HW reset, but for SDA we can try the toggling. For the device not in sync, I wonder how you detect this on bus driver level? If SCL and SDA are high, the device may still be out-of-sync but the bus will look free to the master. Or? > If you are afraid the slave won't see the the STARTs why would it see > any STOPs? Sure, it won't. I can't recall I was afraid of that. Maybe I was not clear what procedure I was proposing: 1) bus is in unkown condition -> send STOP a) good case: bus free, all devices wait for START again b) bad case: SDA stuck low, you can't send STOP 2) SDA is stuck low -> toggle SCL up to 9 times a) good case: SDA is released again, send STOP b) bad case: no SDA release, HW reset needed > Why would a STOP do anything here? The device is not listening until > you get to end of the byte where it will listen to NACK, STOP or > START. And this is a key question, I think: A device should listen to STOP or START always (Specs 3.1.10 Note 4). Do you really have a device that doesn't follow this? Don't get me wrong: I know that there always can be devices out there which do not follow the spec. I'd really love to know if you have one. I'd even like to buy one for testing. But it might be an option to consider this device simply broken. If the solution for this device might mean that other devices get a write to a random location, then there is simply more risk than gain. > The best way to get the slaves attention is the send the 9 clocks with > a START, if possible, in each clock. That will get the slaves > attention and place the slave in START state, then you finish with a > STOP so all devices, including any masters, sees that the bus is > free. Even if I consider there are devices which do not react to START/STOP in the middle of a byte and need it to be fully transferred, I still can't see what a START gains you here. Because it won't react as we just said. After an additional cycle (up to 9 of them), it might react to a STOP then. That I see. And I think I agree of sending a STOP after each of these up to 9 cycles. But I wonder about the start. Note also, that START + STOP (void message) is considered illegal in the standard (Specs 3.1.10 Note 5), although most devices will likely handle it. That's how it looks to me currently. Did I miss something? Regards, Wolfram
On Thu, 2017-06-22 at 12:00 +0200, Wolfram Sang wrote: > On Thu, Jun 22, 2017 at 08:40:25AM +0000, Joakim Tjernlund wrote: > > On Wed, 2017-06-21 at 23:59 +0200, Wolfram Sang wrote: > > > > Toggling 9x even means you could then write something somewhere > > > > which in case of a PMIC can be really dangerous. > > > > > > I am partly wrong here because you send a START beforehand. And devices > > > are required to reset their state machine when they detect a START (I2C > > > Specs 3.1.10, Note 4). So, it *shouldn't* be dangerous. If all devices > > > follow that rule, that is... > > > > > > However, you can only send START when SDA is not stuck. And still, this > > > whole toggling is to reanimate a stuck SDA. So, it still looks to me > > > that it doesn't make sense to have START & STOP around the toggling and > > > rather have a single STOP before you try toggling. > > > > > > Makes sense? > > STOP must be last so the bus is released, this was one of the problems > > the patch fixed as next transfer would find the bus busy and the went > > into fixup again. > > Yes, agreed. STOP must be last. > > > In general you cannot known if the slave stuck in somewhere in > > READ/WRITE or some other state. Stuck is probably the wrong word here, > > not in sync is a better description. > > I think those are two different issues. The bus can be stuck, namely > when SCL or SDA is stuck low. We can't do anything about SCL other than > HW reset, but for SDA we can try the toggling. For the device not in > sync, I wonder how you detect this on bus driver level? If SCL and SDA > are high, the device may still be out-of-sync but the bus will look free > to the master. Or? I don't. I can only find there is some problem and take guess .. > > > If you are afraid the slave won't see the the STARTs why would it see > > any STOPs? > > Sure, it won't. I can't recall I was afraid of that. Maybe I was not > clear what procedure I was proposing: > > 1) bus is in unkown condition -> send STOP > a) good case: bus free, all devices wait for START again > b) bad case: SDA stuck low, you can't send STOP > > 2) SDA is stuck low -> toggle SCL up to 9 times > a) good case: SDA is released again, send STOP > b) bad case: no SDA release, HW reset needed As above, how will you select between these 2 cases? A bit banging driver can but a real controller will have a hard time. > > > Why would a STOP do anything here? The device is not listening until > > you get to end of the byte where it will listen to NACK, STOP or > > START. > > And this is a key question, I think: A device should listen to STOP or > START always (Specs 3.1.10 Note 4). Do you really have a device that > doesn't follow this? > > Don't get me wrong: I know that there always can be devices out there > which do not follow the spec. I'd really love to know if you have one. > I'd even like to buy one for testing. I don't have a device like that, we had a hw disturbance on the bus caused by a i2c mux which were hard to trigger. > > But it might be an option to consider this device simply broken. If the > solution for this device might mean that other devices get a write to a > random location, then there is simply more risk than gain. > > > The best way to get the slaves attention is the send the 9 clocks with > > a START, if possible, in each clock. That will get the slaves > > attention and place the slave in START state, then you finish with a > > STOP so all devices, including any masters, sees that the bus is > > free. > > Even if I consider there are devices which do not react to START/STOP in > the middle of a byte and need it to be fully transferred, I still can't > see what a START gains you here. Because it won't react as we just said. > After an additional cycle (up to 9 of them), it might react to a STOP > then. That I see. And I think I agree of sending a STOP after each of > these up to 9 cycles. But I wonder about the start. Note also, that > START + STOP (void message) is considered illegal in the standard (Specs > 3.1.10 Note 5), although most devices will likely handle it. > > That's how it looks to me currently. Did I miss something? No, this is just speculation of what a device might do. Some will react to only clk, some needs START and/or STOP. In the end of that day we can only do out best and to me a device likelier to react on START than STOP when the device is in a "odd" state. I cannot see how the STARTs can make things worse either. The note of START+STOP is an illegal op. just a note. It does not give slaves a green card to flip over on its back and no device I have seen the last 15 years has done that. Given controller limitations, I2C subsystem should accept fixups that is 9 clks + STOP with 0-9 STARTs in the clk sequence. Jocke
On Thu, May 11, 2017 at 02:20:33PM +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. > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> > Acked-by: Scott Wood <oss@buserror.net> Okay, I admit it is strange to apply a patch after 4 years, but I am doing a bus_recovery overhaul right now and Joakim mentioned a few times, he is still using this patch. I still do wonder why the generic bus recovery algorithm can't be used. It has been updated quite a bit and at least sends STOPs after each pulse (as a result of an earlier discussion about this patch). But a conversion to generic bus recovery wasn't happening, so apply a) what users need and b) document what worked for them. Maybe the conversion will happen somewhen and/or the algorithm here might improve the generic one. We will see. That all being said: Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-mpc.c b/drivers/i2c/busses/i2c-mpc.c index 8393140..6b5e6ce4 100644 --- a/drivers/i2c/busses/i2c-mpc.c +++ b/drivers/i2c/busses/i2c-mpc.c @@ -104,23 +104,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. Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> --- v2 - Remove io barrier call. drivers/i2c/busses/i2c-mpc.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)