[PATCHv2] i2c-mpc: Correct I2C reset procedure

[PATCHv2] 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.

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)
-- 
2.10.2

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

[Joakim Tjernlund]

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)

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

[Joakim Tjernlund]

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)

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

[Scott Wood]

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)
> 
> 

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

[Wolfram Sang]

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

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.


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

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

[Joakim Tjernlund]

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

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

[Wolfram Sang]

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


> 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?


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

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

[Wolfram Sang]

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


> 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?


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

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

[Joakim Tjernlund]

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

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

[Wolfram Sang]

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

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


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

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

[Joakim Tjernlund]

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

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

[Joakim Tjernlund]

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 ...

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

[Wolfram Sang]

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

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!


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