[RFC] i2c-algo-bit: Disable interrupts while SCL is high

[RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Jean Delvare]

Add a spinlock to every user of i2c-algo-bit, which is taken before
raising SCL and released after lowering SCL. We don't really need
the exclusion functionality, but we have to disable local interrupts.
This is needed to comply with SMBus requirements that SCL shouldn't
be high for longer than 50 us.

SMBus slaves can consider SCL being high for 50 us as a timeout
condition. This has been observed to happen reproducibly with the
Melexis MLX90614.

The drawback of this approach is that spin_lock_irqsave() and
spin_unlock_irqrestore() will be called once for each bit going on the
I2C bus in either direction. This can mean up to 100 kHz for standard
I2C and SMBus and up to 250 kHz for fast I2C. The good thing is that
this limits the latency to reasonable values (2us at 250 kHz, 5 us at
100 kHz and 50 us at 10 kHz).

An alternative would be to keep the lock held for the whole transfer
of every single byte. This would divide the number of calls to
spin_lock_irqsave() and spin_unlock_irqrestore() by 9 (i.e. up to 11
kHz for standard I2C and up to 28 kHz for fast I2C) at the price of
multiplying the latency by 18 (i.e. 36 us at 250 kHz, 90 us at 100 kHz
and 900 us at 10 kHz).

I would welcome comments on this. I sincerely have no idea what is
considered a reasonable duration during which local interrupts can be
disabled, and I have also no idea above what frequency taking and
releasing a (never busy) spinlock is considered unreasonable.

i2c-algo-bit is used by many popular I2C and SMBus controller drivers,
so this change will have an effect on virtually every Linux system out
there. So I don't want to get it wrong.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Tested-by: Matthias Zacharias <Matthias.Zacharias@bmk-solutions.de>
---
 drivers/i2c/algos/i2c-algo-bit.c |   38 ++++++++++++++++++++++++++++++++++----
 include/linux/i2c-algo-bit.h     |    4 ++++
 2 files changed, 38 insertions(+), 4 deletions(-)

--- linux-2.6.37-rc6.orig/drivers/i2c/algos/i2c-algo-bit.c	2010-12-16 11:01:39.000000000 +0100
+++ linux-2.6.37-rc6/drivers/i2c/algos/i2c-algo-bit.c	2010-12-16 13:11:12.000000000 +0100
@@ -29,6 +29,7 @@
 #include <linux/sched.h>
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
+#include <linux/spinlock.h>
 
 
 /* ----- global defines ----------------------------------------------- */
@@ -130,12 +131,17 @@ static void i2c_start(struct i2c_algo_bi
 
 static void i2c_repstart(struct i2c_algo_bit_data *adap)
 {
+	unsigned long flags;
+
 	/* assert: scl is low */
 	sdahi(adap);
+	spin_lock_irqsave(&adap->lock, flags);
 	sclhi(adap);
 	setsda(adap, 0);
 	udelay(adap->udelay);
-	scllo(adap);
+	setscl(adap, 0);
+	spin_unlock_irqrestore(&adap->lock, flags);
+	udelay(adap->udelay / 2);
 }
 
 
@@ -163,13 +169,16 @@ static int i2c_outb(struct i2c_adapter *
 	int sb;
 	int ack;
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+	unsigned long flags;
 
 	/* assert: scl is low */
 	for (i = 7; i >= 0; i--) {
 		sb = (c >> i) & 1;
 		setsda(adap, sb);
 		udelay((adap->udelay + 1) / 2);
+		spin_lock_irqsave(&adap->lock, flags);
 		if (sclhi(adap) < 0) { /* timed out */
+			spin_unlock_irqrestore(&adap->lock, flags);
 			bit_dbg(1, &i2c_adap->dev, "i2c_outb: 0x%02x, "
 				"timeout at bit #%d\n", (int)c, i);
 			return -ETIMEDOUT;
@@ -180,10 +189,14 @@ static int i2c_outb(struct i2c_adapter *
 		 * Report a unique code, so higher level code can retry
 		 * the whole (combined) message and *NOT* issue STOP.
 		 */
-		scllo(adap);
+		setscl(adap, 0);
+		spin_unlock_irqrestore(&adap->lock, flags);
+		udelay(adap->udelay / 2);
 	}
 	sdahi(adap);
+	spin_lock_irqsave(&adap->lock, flags);
 	if (sclhi(adap) < 0) { /* timeout */
+		spin_unlock_irqrestore(&adap->lock, flags);
 		bit_dbg(1, &i2c_adap->dev, "i2c_outb: 0x%02x, "
 			"timeout at ack\n", (int)c);
 		return -ETIMEDOUT;
@@ -193,10 +206,13 @@ static int i2c_outb(struct i2c_adapter *
 	 * NAK (usually to report problems with the data we wrote).
 	 */
 	ack = !getsda(adap);    /* ack: sda is pulled low -> success */
+	setscl(adap, 0);
+	spin_unlock_irqrestore(&adap->lock, flags);
+	udelay(adap->udelay / 2);
+
 	bit_dbg(2, &i2c_adap->dev, "i2c_outb: 0x%02x %s\n", (int)c,
 		ack ? "A" : "NA");
 
-	scllo(adap);
 	return ack;
 	/* assert: scl is low (sda undef) */
 }
@@ -209,11 +225,14 @@ static int i2c_inb(struct i2c_adapter *i
 	int i;
 	unsigned char indata = 0;
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+	unsigned long flags;
 
 	/* assert: scl is low */
 	sdahi(adap);
 	for (i = 0; i < 8; i++) {
+		spin_lock_irqsave(&adap->lock, flags);
 		if (sclhi(adap) < 0) { /* timeout */
+			spin_unlock_irqrestore(&adap->lock, flags);
 			bit_dbg(1, &i2c_adap->dev, "i2c_inb: timeout at bit "
 				"#%d\n", 7 - i);
 			return -ETIMEDOUT;
@@ -222,6 +241,7 @@ static int i2c_inb(struct i2c_adapter *i
 		if (getsda(adap))
 			indata |= 0x01;
 		setscl(adap, 0);
+		spin_unlock_irqrestore(&adap->lock, flags);
 		udelay(i == 7 ? adap->udelay / 2 : adap->udelay);
 	}
 	/* assert: scl is low */
@@ -384,16 +404,21 @@ static int sendbytes(struct i2c_adapter
 static int acknak(struct i2c_adapter *i2c_adap, int is_ack)
 {
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+	unsigned long flags;
 
 	/* assert: sda is high */
 	if (is_ack)		/* send ack */
 		setsda(adap, 0);
 	udelay((adap->udelay + 1) / 2);
+	spin_lock_irqsave(&adap->lock, flags);
 	if (sclhi(adap) < 0) {	/* timeout */
+		spin_unlock_irqrestore(&adap->lock, flags);
 		dev_err(&i2c_adap->dev, "readbytes: ack/nak timeout\n");
 		return -ETIMEDOUT;
 	}
-	scllo(adap);
+	setscl(adap, 0);
+	spin_unlock_irqrestore(&adap->lock, flags);
+	udelay(adap->udelay / 2);
 	return 0;
 }
 
@@ -616,6 +641,11 @@ static int __i2c_bit_add_bus(struct i2c_
 	adap->algo = &i2c_bit_algo;
 	adap->retries = 3;
 
+	/* We use a spinlock to block interrupts while SCL is high.
+	 * Otherwise the very short SMBus SCL high timeout (50 us)
+	 * can be reached, causing SMBus slaves to stop responding. */
+	spin_lock_init(&bit_adap->lock);
+
 	ret = add_adapter(adap);
 	if (ret < 0)
 		return ret;
--- linux-2.6.37-rc6.orig/include/linux/i2c-algo-bit.h	2010-12-16 11:00:59.000000000 +0100
+++ linux-2.6.37-rc6/include/linux/i2c-algo-bit.h	2010-12-16 13:11:41.000000000 +0100
@@ -24,6 +24,8 @@
 #ifndef _LINUX_I2C_ALGO_BIT_H
 #define _LINUX_I2C_ALGO_BIT_H
 
+#include <linux/spinlock.h>
+
 /* --- Defines for bit-adapters ---------------------------------------	*/
 /*
  * This struct contains the hw-dependent functions of bit-style adapters to
@@ -39,6 +41,8 @@ struct i2c_algo_bit_data {
 	int  (*pre_xfer)  (struct i2c_adapter *);
 	void (*post_xfer) (struct i2c_adapter *);
 
+	spinlock_t lock;	/* Disable interrupts when SCL is high */
+
 	/* local settings */
 	int udelay;		/* half clock cycle time in us,
 				   minimum 2 us for fast-mode I2C,


-- 
Jean Delvare

Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Ben Dooks]

On Thu, Dec 16, 2010 at 03:06:38PM +0100, Jean Delvare wrote:
> Add a spinlock to every user of i2c-algo-bit, which is taken before
> raising SCL and released after lowering SCL. We don't really need
> the exclusion functionality, but we have to disable local interrupts.
> This is needed to comply with SMBus requirements that SCL shouldn't
> be high for longer than 50 us.
> 
> SMBus slaves can consider SCL being high for 50 us as a timeout
> condition. This has been observed to happen reproducibly with the
> Melexis MLX90614.
> 
> The drawback of this approach is that spin_lock_irqsave() and
> spin_unlock_irqrestore() will be called once for each bit going on the
> I2C bus in either direction. This can mean up to 100 kHz for standard
> I2C and SMBus and up to 250 kHz for fast I2C. The good thing is that
> this limits the latency to reasonable values (2us at 250 kHz, 5 us at
> 100 kHz and 50 us at 10 kHz).

Hmm, this is going to be a drain on interrupt latency... disabling
interrupts in a system for that long could cause other things to
jitter.

I think if there's a time constraint, we should look at a method of
using a high-resolution timer to run the clocks so that we don't
have to wait around polling stuff.
 
> An alternative would be to keep the lock held for the whole transfer
> of every single byte. This would divide the number of calls to
> spin_lock_irqsave() and spin_unlock_irqrestore() by 9 (i.e. up to 11
> kHz for standard I2C and up to 28 kHz for fast I2C) at the price of
> multiplying the latency by 18 (i.e. 36 us at 250 kHz, 90 us at 100 kHz
> and 900 us at 10 kHz).
> 
> I would welcome comments on this. I sincerely have no idea what is
> considered a reasonable duration during which local interrupts can be
> disabled, and I have also no idea above what frequency taking and
> releasing a (never busy) spinlock is considered unreasonable.

The cost of IRQ-spinlock on UP-ARM is about 4 instructions for each lock
and unlock. So taking it a-lot isn't costly in this place... not sure
for the MP variants.
 
>  /* ----- global defines ----------------------------------------------- */
> @@ -130,12 +131,17 @@ static void i2c_start(struct i2c_algo_bi
>  
>  static void i2c_repstart(struct i2c_algo_bit_data *adap)
>  {
> +	unsigned long flags;
> +
>  	/* assert: scl is low */
>  	sdahi(adap);
> +	spin_lock_irqsave(&adap->lock, flags);
>  	sclhi(adap);
>  	setsda(adap, 0);
>  	udelay(adap->udelay);
> -	scllo(adap);
> +	setscl(adap, 0);
> +	spin_unlock_irqrestore(&adap->lock, flags);
> +	udelay(adap->udelay / 2);
>  }

would be nice to document why we're taking this lock here... or in the
header add some more explanation other than 'whilst clock is high'

anyway, the rest looks fine from reading through, there's no obvious
problems.

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Jean Delvare]

Hi Ben,

On Thu, 16 Dec 2010 16:00:46 +0000, Ben Dooks wrote:
> On Thu, Dec 16, 2010 at 03:06:38PM +0100, Jean Delvare wrote:
> > Add a spinlock to every user of i2c-algo-bit, which is taken before
> > raising SCL and released after lowering SCL. We don't really need
> > the exclusion functionality, but we have to disable local interrupts.
> > This is needed to comply with SMBus requirements that SCL shouldn't
> > be high for longer than 50 us.
> > 
> > SMBus slaves can consider SCL being high for 50 us as a timeout
> > condition. This has been observed to happen reproducibly with the
> > Melexis MLX90614.
> > 
> > The drawback of this approach is that spin_lock_irqsave() and
> > spin_unlock_irqrestore() will be called once for each bit going on the
> > I2C bus in either direction. This can mean up to 100 kHz for standard
> > I2C and SMBus and up to 250 kHz for fast I2C. The good thing is that
> > this limits the latency to reasonable values (2us at 250 kHz, 5 us at
> > 100 kHz and 50 us at 10 kHz).
> 
> Hmm, this is going to be a drain on interrupt latency... disabling
> interrupts in a system for that long could cause other things to
> jitter.

So you consider that even disabling interrupts for 5 us is too long? Or
are you only worried by the 50 us case?

> I think if there's a time constraint, we should look at a method of
> using a high-resolution timer to run the clocks so that we don't
> have to wait around polling stuff.

Good suggestion. Are you willing to try and implement this yourself? I
am not familiar with high resolution timers.

Another possibility would be to make the spinlock usage optional. Only
SMBus slaves care about the timeout, I2C slaves do not, and not all
SMBus slaves are as sensitive as the MLX90614. I didn't want to make it
optional at first because it will make the code even more bloated, but
if you really think that disabling interrupts for 2 to 50 us will cause
problems in practice, I could look into it again.

> > An alternative would be to keep the lock held for the whole transfer
> > of every single byte. This would divide the number of calls to
> > spin_lock_irqsave() and spin_unlock_irqrestore() by 9 (i.e. up to 11
> > kHz for standard I2C and up to 28 kHz for fast I2C) at the price of
> > multiplying the latency by 18 (i.e. 36 us at 250 kHz, 90 us at 100 kHz
> > and 900 us at 10 kHz).

If you consider even per-bit locking as too high latency, I guess that
this alternative proposal is out of the question?

> > I would welcome comments on this. I sincerely have no idea what is
> > considered a reasonable duration during which local interrupts can be
> > disabled, and I have also no idea above what frequency taking and
> > releasing a (never busy) spinlock is considered unreasonable.
> 
> The cost of IRQ-spinlock on UP-ARM is about 4 instructions for each lock
> and unlock. So taking it a-lot isn't costly in this place... not sure
> for the MP variants.
>  
> >  /* ----- global defines ----------------------------------------------- */
> > @@ -130,12 +131,17 @@ static void i2c_start(struct i2c_algo_bi
> >  
> >  static void i2c_repstart(struct i2c_algo_bit_data *adap)
> >  {
> > +	unsigned long flags;
> > +
> >  	/* assert: scl is low */
> >  	sdahi(adap);
> > +	spin_lock_irqsave(&adap->lock, flags);
> >  	sclhi(adap);
> >  	setsda(adap, 0);
> >  	udelay(adap->udelay);
> > -	scllo(adap);
> > +	setscl(adap, 0);
> > +	spin_unlock_irqrestore(&adap->lock, flags);
> > +	udelay(adap->udelay / 2);
> >  }
> 
> would be nice to document why we're taking this lock here... or in the
> header add some more explanation other than 'whilst clock is high'

The comment is where the spinlock is initialized:

	/* We use a spinlock to block interrupts while SCL is high.
	 * Otherwise the very short SMBus SCL high timeout (50 us)
	 * can be reached, causing SMBus slaves to stop responding. */
	spin_lock_init(&bit_adap->lock);

Do you consider this insufficient, or do you simply think it should be
located somewhere else?

> anyway, the rest looks fine from reading through, there's no obvious
> problems.

Thanks for the review.

-- 
Jean Delvare

Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Michael Lawnick]

Jean Delvare said the following:
> Hi Ben,
> 
> On Thu, 16 Dec 2010 16:00:46 +0000, Ben Dooks wrote:
>> On Thu, Dec 16, 2010 at 03:06:38PM +0100, Jean Delvare wrote:
>> > Add a spinlock to every user of i2c-algo-bit, which is taken before
>> > raising SCL and released after lowering SCL. We don't really need
>> > the exclusion functionality, but we have to disable local interrupts.
>> > This is needed to comply with SMBus requirements that SCL shouldn't
>> > be high for longer than 50 us.
>> > 
>> > SMBus slaves can consider SCL being high for 50 us as a timeout
>> > condition. This has been observed to happen reproducibly with the
>> > Melexis MLX90614.
>> > 
>> > The drawback of this approach is that spin_lock_irqsave() and
>> > spin_unlock_irqrestore() will be called once for each bit going on the
>> > I2C bus in either direction. This can mean up to 100 kHz for standard
>> > I2C and SMBus and up to 250 kHz for fast I2C. The good thing is that
>> > this limits the latency to reasonable values (2us at 250 kHz, 5 us at
>> > 100 kHz and 50 us at 10 kHz).
>> 
>> Hmm, this is going to be a drain on interrupt latency... disabling
>> interrupts in a system for that long could cause other things to
>> jitter.
> 
> So you consider that even disabling interrupts for 5 us is too long? Or
> are you only worried by the 50 us case?

Sorry to disturb, but
<MANTRA>
	Disabling interrupts may be done only for a few instructions.</MANTRA>

Even 1 us is an eternity on modern systems.

JM2C

-- 
KR
Michael

Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Jean Delvare]

Hi Michael,

On Fri, 17 Dec 2010 13:09:54 +0100, Michael Lawnick wrote:
> Jean Delvare said the following:
> > Hi Ben,
> > 
> > On Thu, 16 Dec 2010 16:00:46 +0000, Ben Dooks wrote:
> >> On Thu, Dec 16, 2010 at 03:06:38PM +0100, Jean Delvare wrote:
> >> > Add a spinlock to every user of i2c-algo-bit, which is taken before
> >> > raising SCL and released after lowering SCL. We don't really need
> >> > the exclusion functionality, but we have to disable local interrupts.
> >> > This is needed to comply with SMBus requirements that SCL shouldn't
> >> > be high for longer than 50 us.
> >> > 
> >> > SMBus slaves can consider SCL being high for 50 us as a timeout
> >> > condition. This has been observed to happen reproducibly with the
> >> > Melexis MLX90614.
> >> > 
> >> > The drawback of this approach is that spin_lock_irqsave() and
> >> > spin_unlock_irqrestore() will be called once for each bit going on the
> >> > I2C bus in either direction. This can mean up to 100 kHz for standard
> >> > I2C and SMBus and up to 250 kHz for fast I2C. The good thing is that
> >> > this limits the latency to reasonable values (2us at 250 kHz, 5 us at
> >> > 100 kHz and 50 us at 10 kHz).
> >> 
> >> Hmm, this is going to be a drain on interrupt latency... disabling
> >> interrupts in a system for that long could cause other things to
> >> jitter.
> > 
> > So you consider that even disabling interrupts for 5 us is too long? Or
> > are you only worried by the 50 us case?
> 
> Sorry to disturb, but
> <MANTRA>
> 	Disabling interrupts may be done only for a few instructions.</MANTRA>
> 
> Even 1 us is an eternity on modern systems.

Don't be sorry, this is exactly the kind of input I was asking for. I'm
a little surprised, I thought disabling interrupts for a couple
microseconds was happening all the time, but I'll trust your
experience. Given your point and Ben's, it seems clear that my patch is
not acceptable as is, and at the very least I should make the spinlock
usage optional.

High-resolution timers may be an option too, but I guess it will
require a rewrite of the driver, and also I don't think HR timers are
available everywhere, so we will have to keep the old code in place for
compatibility.

Matthias, can you please tell us whether your system supports
high-resolution timers? I need to know if that would be a viable
solution for you.

-- 
Jean Delvare

Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Michael Lawnick]

Jean Delvare said the following:
> Hi Michael,
> 
> On Fri, 17 Dec 2010 13:09:54 +0100, Michael Lawnick wrote:
>> Sorry to disturb, but
>> <MANTRA>
>> 	Disabling interrupts may be done only for a few instructions.</MANTRA>
>> 
>> Even 1 us is an eternity on modern systems.
> 
> Don't be sorry, this is exactly the kind of input I was asking for. I'm
> a little surprised, I thought disabling interrupts for a couple
> microseconds was happening all the time, but I'll trust your
> experience. 

I can't tell whether this is happening all the time, but I can imagine
and I highly discourage this. This is IMHO one of the lessons many LINUX
developers have still to learn. Maybe it's a history reason.

> Given your point and Ben's, it seems clear that my patch is
> not acceptable as is, and at the very least I should make the spinlock
> usage optional.

At last you might not come around your solution, but a H/W-S/W
combination driving you in such a direction should be considered broken.
Using it in professional environment needs heavy discussions about pros
and cons, best would be to beat the H/W designers to provide a real
controller.
Of course it may be used in a case, where you simply need a (temporary)
hack to get something working.

-- 
KR
Michael

Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Jean Delvare]

Michael,

On Mon, 20 Dec 2010 08:24:07 +0100, Michael Lawnick wrote:
> Jean Delvare said the following:
> > Hi Michael,
> > 
> > On Fri, 17 Dec 2010 13:09:54 +0100, Michael Lawnick wrote:
> >> Sorry to disturb, but
> >> <MANTRA>
> >> 	Disabling interrupts may be done only for a few instructions.</MANTRA>
> >> 
> >> Even 1 us is an eternity on modern systems.
> > 
> > Don't be sorry, this is exactly the kind of input I was asking for. I'm
> > a little surprised, I thought disabling interrupts for a couple
> > microseconds was happening all the time, but I'll trust your
> > experience. 
> 
> I can't tell whether this is happening all the time, but I can imagine
> and I highly discourage this. This is IMHO one of the lessons many LINUX
> developers have still to learn. Maybe it's a history reason.
> 
> > Given your point and Ben's, it seems clear that my patch is
> > not acceptable as is, and at the very least I should make the spinlock
> > usage optional.
> 
> At last you might not come around your solution, but a H/W-S/W
> combination driving you in such a direction should be considered broken.

I don't disagree, but the fact is that i2c-algo-bit is frequently used.
It's used by all framebuffer and KMS drivers which want to read the
EDID data from the screen. It's also used by a large number of V4L and
DVB drivers.

Note that, in the cases above, the current i2c-algo-bit implementation
should be OK. These are cards with I2C devices mostly, not SMBus
devices, so they are much more tolerant to slow SCL.

> Using it in professional environment needs heavy discussions about pros
> and cons, best would be to beat the H/W designers to provide a real
> controller.

For future designs, certainly. But we also have to deal with the
existing hardware.

> Of course it may be used in a case, where you simply need a (temporary)
> hack to get something working.

The problem is that "temporary" tends to stay, either because there is
no other way, or because the hardware implementation is unreliable or
limited, or because we don't have the specifications of the hardware
implementation. So I don't see i2c-algo-bit going away anytime soon.

-- 
Jean Delvare

Antw: Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Matthias Zacharias]

Hi Jean,

Sorry for the late answer I was in christmas holidays.

>>> Jean Delvare <khali@linux-fr.org> schrieb am Samstag, 18. Dezember
2010 um
00:09 in Nachricht <20101218000924.546ad703@endymion.delvare>:
> Hi Michael,
> 
> On Fri, 17 Dec 2010 13:09:54 +0100, Michael Lawnick wrote:
>> Jean Delvare said the following:
>> > Hi Ben,
>> > 
>> > On Thu, 16 Dec 2010 16:00:46 +0000, Ben Dooks wrote:
>> >> On Thu, Dec 16, 2010 at 03:06:38PM +0100, Jean Delvare wrote:
>> >> > Add a spinlock to every user of i2c-algo-bit, which is taken
before
>> >> > raising SCL and released after lowering SCL. We don't really
need
>> >> > the exclusion functionality, but we have to disable local
interrupts.
>> >> > This is needed to comply with SMBus requirements that SCL
shouldn't
>> >> > be high for longer than 50 us.
>> >> > 
>> >> > SMBus slaves can consider SCL being high for 50 us as a
timeout
>> >> > condition. This has been observed to happen reproducibly with
the
>> >> > Melexis MLX90614.
>> >> > 
>> >> > The drawback of this approach is that spin_lock_irqsave() and
>> >> > spin_unlock_irqrestore() will be called once for each bit going
on the
>> >> > I2C bus in either direction. This can mean up to 100 kHz for
standard
>> >> > I2C and SMBus and up to 250 kHz for fast I2C. The good thing is
that
>> >> > this limits the latency to reasonable values (2us at 250 kHz, 5
us at
>> >> > 100 kHz and 50 us at 10 kHz).
>> >> 
>> >> Hmm, this is going to be a drain on interrupt latency...
disabling
>> >> interrupts in a system for that long could cause other things to
>> >> jitter.
>> > 
>> > So you consider that even disabling interrupts for 5 us is too
long? Or
>> > are you only worried by the 50 us case?
>> 
>> Sorry to disturb, but
>> <MANTRA>
>> 	Disabling interrupts may be done only for a few
instructions.</MANTRA>
>> 
>> Even 1 us is an eternity on modern systems.
> 
> Don't be sorry, this is exactly the kind of input I was asking for.
I'm
> a little surprised, I thought disabling interrupts for a couple
> microseconds was happening all the time, but I'll trust your
> experience. Given your point and Ben's, it seems clear that my patch
is
> not acceptable as is, and at the very least I should make the
spinlock
> usage optional.
> 
> High-resolution timers may be an option too, but I guess it will
> require a rewrite of the driver, and also I don't think HR timers
are
> available everywhere, so we will have to keep the old code in place
for
> compatibility.
> 
> Matthias, can you please tell us whether your system supports
> high-resolution timers? I need to know if that would be a viable
> solution for you.

I don't know if in the kernel 2.6.25.4 high-resolution counters are
supported for the ARM processor we use (AT91SAM9261), but if such a
solution is possible we should evaluate.it.
We have to consider the following aspects using "i2c-gpio" and
"i2c-algo-bit" driver:
     1. the hardware interface inplemented by ATMEL for SMB and I2C
communication (TWI) can't be configured to be conform with the SMBus
spezification 2.0. As result we have to use these drivers
     2. the "i2c-algo-bit" functions drives directly the port pins via
"i2c-gpio" are interrupted by HW-ISR routines and as softinterrupt
service routines run in the kernel. Both lead to an unpredictable timing
in the SMBus communication (see the screenshots in my dropbox:
http://www.dropbox.com/gallery/16457261/1/I2C_2_MLX90614?h=8e2a46).
It is nearly impossible to find the reasons for these clock-strechings.
My system shows only 4 intterrupts (the frequent 2 are network- and usb-
subsystem)  sources but these can't be disabled because the system gets
unfunctional.
In my opinion "i2c-algo-bit" and "i2c-gpio" drivers should be used only
if there is no or unfunctional hardware support for i2c and SMBus
available, otherwise the drivers usings hardware support should be
prefered.

--------------------
BMK electronic solut
ions GmbH
Werner-von-Siemens-Str. 6, Eingang 18 f
D-86159 Augsburg
Tel. +49 (0) 821 / 207 88 - 700
Fax +49 (0) 821 / 207 88 - 721
info@bmk-solutions.de
Geschäftsführer: Dipl.-oec. Alois Knöferle
Sitz: Augsburg
HR-Nr.: B21197
---------------------

Diese E-mail kann vertrauliche Informationen enthalten. Falls Sie diese
E-Mail irrtümlich erhalten haben, informieren Sie bitte unverzüglich den
Absender und löschen Sie diese E-Mail von jedem Rechner, auch von den
Mailservern. Jede Verbreitung des Inhalts, auch die teilweise
Verbreitung, ist in diesem Fall untersagt. Außer bei Vorsatz oder grober
Fahrlässigkeit schliessen wir jegliche Haftung für Verluste oder Schäden
aus, die durch Viren befallene Software oder E-Mails verursacht werden.

This e-mail may contain confidential information. If you received this
e-mail in error, please contact the sender and delete this e-mail from
your computer, including your mailservers. Any dissemination, even
partly, is prohibited. Except in case of gross negligence or wilful
misconduct we accept no liability for any loss or damage caused by
software or e-mail viruses.

Re: [RFC] i2c-algo-bit: Disable interrupts while SCL is high

[Jean Delvare]

Hi Michael, Ben,

On Fri, 17 Dec 2010 13:09:54 +0100, Michael Lawnick wrote:
> Jean Delvare said the following:
> > Hi Ben,
> > 
> > On Thu, 16 Dec 2010 16:00:46 +0000, Ben Dooks wrote:
> >> On Thu, Dec 16, 2010 at 03:06:38PM +0100, Jean Delvare wrote:
> >> > Add a spinlock to every user of i2c-algo-bit, which is taken before
> >> > raising SCL and released after lowering SCL. We don't really need
> >> > the exclusion functionality, but we have to disable local interrupts.
> >> > This is needed to comply with SMBus requirements that SCL shouldn't
> >> > be high for longer than 50 us.
> >> > 
> >> > SMBus slaves can consider SCL being high for 50 us as a timeout
> >> > condition. This has been observed to happen reproducibly with the
> >> > Melexis MLX90614.
> >> > 
> >> > The drawback of this approach is that spin_lock_irqsave() and
> >> > spin_unlock_irqrestore() will be called once for each bit going on the
> >> > I2C bus in either direction. This can mean up to 100 kHz for standard
> >> > I2C and SMBus and up to 250 kHz for fast I2C. The good thing is that
> >> > this limits the latency to reasonable values (2us at 250 kHz, 5 us at
> >> > 100 kHz and 50 us at 10 kHz).
> >> 
> >> Hmm, this is going to be a drain on interrupt latency... disabling
> >> interrupts in a system for that long could cause other things to
> >> jitter.
> > 
> > So you consider that even disabling interrupts for 5 us is too long? Or
> > are you only worried by the 50 us case?
> 
> Sorry to disturb, but
> <MANTRA>
> 	Disabling interrupts may be done only for a few instructions.</MANTRA>
> 
> Even 1 us is an eternity on modern systems.

Well well, no matter how little you and Ben seem to like it, it appears
that w1 is going more or less the route I had proposed for i2c-algo-bit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=commitdiff;h=3fd306c85adcde7209281cb663dd8ea247e97cc3

So I am again considering something similar for i2c (at least as an
option.) Presumably their local_irq_save() is cheaper than my
spin_lock_irqsave(), so I'd use that, not sure why I didn't do it
originally.

-- 
Jean Delvare