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 <[email protected]>
Tested-by: Matthias Zacharias <[email protected]>
---
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
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, [email protected], http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
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
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
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
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
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