root/i2c/trunk/TODO @ 5363

I2C / SMBus TODO list
Contact us if you have comments or wish to help.
------------------------------------------------

* Only chip driver modules are locked in memory when one accesses their
  /proc entries. Bus drivers are never locked (i.e. no reference count
  is done). Bus drivers should be locked by the simple fact that a client
  uses them (which would make it impossible to unload them before
  all using client drivers have been unloaded themselves). This might be
  restricted to clients that actually have /proc entries.

* "uninstall" Makefile target.

* i2c-algo-bit problems, and unfinished i2c-algo-biths
  See the following two entries marked D.E. (Dori Eldar's suggestions),
  and see the email thread at the bottom of this file.

* Timing considerations in SMBus emulation with i2c-algo-bit (D.E.):
  (Note that some of these changes are implemented in i2c-algo-biths)
  The SMBus defines a minimum frequency of 10 kHz for driving the bus, while
  the I2C does not define any minimum frequency.
  furthermore the maximum time a master is allowed to keep the CLK line high
  is 50 usec. this is required so masters can detect an idle bus
  (the CLK line is high for at least 50 usec), again the I2C does not impose
  this restriction.
  It's crucial that masters will obey the last timing consideration, since:
        1. Slaves may otherwise hang the transaction.
        2. Other masters will assume the bus is idle, initiate their own SMBus
           transaction, which will lead to corruption of data
           carried over the SMBus.
  Note that a correct arbitration procedure can take
  place only if masters are "synchronized" meaning, they initiate the
  transaction at the _same_ time.

  Now when implementing the SMBus protocol in SW one has to make sure that in
  the critical sections in which the CLK is held high, SW is not preempted.
  Or more simply: SW must disable interrupts at this period of time.
  This critical periods are:
        a. Waiting for an idle bus before starting the transaction until the
           CLK is driven low after generating the START signal.
        b. For each clock cycle in the transaction: before the CLK is set
           to HIGH till after the CLK is set LOW.

  Looking at the i2c-algo-bit.c it's very problematic to add the interrupt
  disabling code since the code may sleep in the critical period at the
  "sclhi" function.
  You would also need to add a "wait for idle bus" before the driving the
  START signal. 

* Arbitration in SMBus emulation with i2c-algo-bit (D.E.):
  Although the I2C disallows arbitration between masters while one is sending
  a restart signal & another sending a data signal, this situation is
  theoretically possible on an SMBus.
  So you would need to check for arbitration when driving the restart signal
  too. BTW why is the arbitration check code disabled in the i2c_outb?

* Pre-Post routines with i2c-algo-bit (D.E.): 
  Before & after driving an Smbus transaction, if HW must be accessed, such
  access is not possible with current algo-bit design. It's possible to allow
  such access by doing
  an EXPORT_SYMBOL(i2c_bit_xfer) therefore making the algo-bit available as a
  library function only. Or by adding a post & pre function pointers to the
  algo-bit structure.

* 16-bit Register Addresses
  There is no support for 16-bit register addresses (used by serial
  eeproms larger than 16K bit, such as the Atmel 24C32) in i2c-core.c
  or i2c-dev.c.
  Emulation support is required.
  General 16-bit support for all transaction types will require
  many changes.
  Note that writes with an arbitrary number of address bytes
  are actually supported now by treating the extra bytes as
  the beginning of the data.
  There is an alternate proposal from Wolfgang Denk <wd@denx.de>
  for a driver 'i2c-simple' that supports a generalized address
  length of 1-4 bytes and accesses via ioctls on /dev.

* Host-as-slave mode in i2c adapters may need to be integrated
  with the adapter host code, because the lm_sensors-type chip driver
  architecture is not well suited to implementing a chip slave,
  the slave mode will require locking with the master mode, and
  specialized commmunication such as "Notify ARP Master".
  Adapters may respond to one or more host-as-slave addresses. The
  functionality bits and the rest of the API will have to be extended
  to support slaves embedded in host adapters.

* 64-bit functionality values may be required to represent all
  the new capabilities described above.

* The Maximum SMBus block transfer size is 32 bytes,
  as defined in the SMBus specification, and
  assumption is scattered throughout the code. Replace hardcoded '32'
  throughout the code with I2C_SMBUS_BLOCK_MAX to be clear.

* Emulation layer i2c block reads are fixed at 32 bytes and there is
  currently no method to change it. You cannot, for example,
  read an entire serial eeprom with a single block read, or read
  only the 7 bytes in a clock chip. There is no i2c limitation on
  block size but the code is designed for the 32 byte limit of
  SMBus.

* i2c version strings were added to i2c.h but they are used only
  for printk's. Integers would be better for use in preprocessor
  directives for conditional compiles.

* Alternative i2c implementations in kernel to be converted to
  the standard i2c implementation in this package.
  Most if not all of these are bit-banging algorithms,
  for which the official driver is drivers/i2c/i2c-algo-bit.c.
  For a good example of using i2c-algo-bit, see drivers/acorn/char/i2c.c.
  i2c-old.h was removed in kernel 2.5.1.

        drivers/media/video/i2c-old.c   Used by:
                drivers/media/video/buz.c
                drivers/media/video/i2c-parport.c
                drivers/media/video/saa7110.c
                drivers/media/video/saa7111.c
                drivers/media/video/saa7185.c
                drivers/media/video/stradis.c
                drivers/media/video/zr36120.c
                drivers/media/video/zr36120_i2c.c
        arch/ppc/mbxboot/iic.c
        drivers/media/radio/radio-trust.c
        drivers/media/video/pms.c
        drivers/media/video/stradis.c
        drivers/net/acenic.c
        drivers/net/sk98lin/ski2c.c
        drivers/sbus/char/envctrl.c
        drivers/sbus/char/vfc_i2c.c
        drivers/scsi/cpqfcTSi2c.c
        drivers/usb/ov511.c

* Make especially i2c-core SMP-safe. This means: locks around all global
  variable access, especially during (de)registration activity.

* Check debugging levels in all modules are sane.

* linux/Documentation/devices.txt lists i2c0, i2c1 etc. instead of i2c-0, i2c-1

* At least the bit-lp and bit-velle modules do no detection on loading;
  ask Simon whether this is possible to add.

* Correct all module locking code (see Keith Owens' email about this)

* i2c-algo-bit problems, and unfinished i2c-algo-biths
  Below is a reformatted email thread between Kyosti Malkki and MDS,
  November-December 2002, on the lm_sensors mailing list.
  This discussion identifies timing and multi-master
  problems in i2c-algo-bit. Kyosti wrote a new driver, i2c-algo-biths,
  which attempts to fix these problems. The new driver has not been
  heavily tested and may be incomplete; the old driver still has
  the problems identified below:

=======================================================================
Kyosti Malkki wrote:

> 
> I have no scope around to check this, but looking into i2c-algo-bit.c,
> adap->udelay is used where it is not necessary. As an example, udelay=3
> would look roughly like 25/75 duty cycle:
> 
> SDA: -._________---._________---.------------.-
> SCL: _.___---______.___---______.___---______.-
>         ^     ^  ^   ^     ^  ^   ^     ^  ^
> 
> Dots just point out where sw goes for next bit, others 1 us wide each.
> That is, all bus transitions use adap->udelay, where a rise/fall delay
> of 1 us (or less) would do.
> 
> Removing the unnecessary 2us marked ^ _above_ :
> 
> SDA: -._____-._____-.------.-
> SCL: _._---__._---__._---__.-
>             ^ ^      ^    ^
> 
> When successive bits to put on bus are equal, SDA rise/fall may be
> omitted.
> 
> SDA: -._____.____-.-----.-
> SCL: _._---_.---__.---__.-
> 
> Interrupt service and PCI latency may add to the delay time.  Any
> decrease is not possible, if write-read sequence is used in
> setscl() and setsda(). Right ?
> 
> In the process I tested using nominal 1us delay everywhere, and
> i2c_adap->udelay only to clock bits in and out.  I believe it does not
> violate specs, but will likely fail with higher bus capacitance.
> This changes duty-cycle and might break some support, so I won't commit
> it now. It does improve the speed upto 333 kbps, using adap->udelay=1.
> 

=======================================================================
MDS:

The last time I used a scope on the i2c bus (quite a while ago), I was
surprised to see that
the clock was not a 50/50 duty cycle. But that's what the code does.
I don't think it's correct, or at least it isn't optimal.

If I'm looking at your 2nd picture correctly, it looks like it
implements
a 50/50 duty cycle. That looks right to me.
I don't understand why there are udelays() in the sdalo() and sdahi()
functions.

I just recently added some comments to i2c-algo-bit.h about the units,
(I kept forgetting myself, and others would ask periodically too)
and made some fixes to the drivers to handle HZ != 100.

What the comment I added says, and it isn't strictly true,
is that clock rate = 500,000 / udelay. It does mean that
the minimum half-cycle time (either high _or_ low) is == udelay.

Chip datasheets generally specify maximum I2C clock rate.
They _dont_ specify minimums for clock low or clock high,
it is implied to be == 1 / (2 *  maximum rate).

We don't want to violate that. If udelay == 5, 
(which implies a 100 kHz clock) that should
be the minimum time high _or_ low for the clock.
So I don't think your third picture below should be implemented.

I agree that any latency assurance should be in the setsda() and
setscl() functions.

=======================================================================
KM:

On Wed, 27 Nov 2002, Mark D. Studebaker wrote:

> If I'm looking at your 2nd picture correctly, it looks like it
> implements a 50/50 duty cycle. That looks right to me. I don't
> understand why there are udelays() in the sdalo() and sdahi()
> functions.

It was udelay/3 duty cycle. Some setup and hold time is required,
1 us is around 66 PCI clocks.

> We don't want to violate that. If udelay == 5,
> (which implies a 100 kHz clock) that should
> be the minimum time high _or_ low for the clock.

If we use 1us in sdahi/lo, and udelay=3 for sclhi/lo,
we get duty cycle of udelay/(udelay+2):

SDA: -__--------_______---------
SCL: --__---_____---_____---____

Finally, with SDA half of SCL clock rate and (udelay-1) offset:

SDA: -______________-------------______
SCL: --___---___---___---___---___---___

As for other issues in algo-bit:

At several locations, return code of sclhi() is silently ignored,

I think I replace i2c_start with i2c_repstart -- if SCL is already low,
we may have multi-master or bus stuck. To count for possibility of a
second master, we should read back SDA level on master write slots.

Are return values of i2c_transfer and friends explained somewhere?


=======================================================================
MDS:

I looked carefully at the loops in i2c_outb() and i2c_inb().
These are what set the timing and duty cycle for most of an i2c bus
cycle.
I'm (for the moment) ignoring timing and issues outside these loops.
If any of these statements don't look correct, speak up :)

- i2c_outb generates a 33% duty cycle clock (udelay high, 2 * udelay
low)
- i2c_inb generates a 50/50 clock (udelay high, udelay low)
- Neither function calls sdahi or sdalo inside its loop in normal
operation,
   so changes to sdahi/sdalo won't affect the duty cycle.
- It appears that we can make changes to i2c_outb to get a 50/50 clock.
  The loop from i2c_outb() is annotated below. Note that these
  changes don't correctly handle delays coming into and out of the loop.



        /* assert: scl is low */
        for ( i=7 ; i>=0 ; i-- ) {
                sb = c & ( 1 << i );
                setsda(adap,sb);
// The following udelay ensures setup time to the r.e. of the clock,
// and ensures the clock low time (together with the udelay at the
bottom of the loop)
// It could be changed to udelay(adap->udelay - 1)
                udelay(adap->udelay);
                DEBPROTO(printk(KERN_DEBUG "%d",sb!=0));
// The following call to sclhi() contains a call to delay(adap->udelay)
// which ensures the clock high time
                if (sclhi(adap)<0) { /* timed out */
                        sdahi(adap); /* we don't want to block the net */
                        DEB2(printk(KERN_DEBUG " i2c_outb: 0x%02x, timeout at bi                        
                        return -ETIMEDOUT;
                };
                /* do arbitration here: 
                 * if ( sb && ! getsda(adap) ) -> ouch! Get out of here.
                 */
                setscl(adap, 0 );
// The following udelay ensures hold time after the f.e. of the clock,
// and ensures the clock low time (together with the udelay at the top
of the loop)
// It could be changed to udelay(1) and still meet the 300 ns hold time
// (ref. I2C bus spec version 2.1, table 5, note 2)
                udelay(adap->udelay);
        }

=======================================================================
KM:

Unfortunately sclhi() does not ensure clock high time the right way.
The delay should be after we read clock as high.

Usually this means we get no acknowledge from the client, since it has
missed this one clock cycle. If we miss a bit during the address bits,
we may access incorrect chips or more than one chip at a time.


=======================================================================
KM:
(comments about his i2c-algo-biths patch)

Some notes to comment on the changes in the patch..

I have been reading the "Fast Mode" specs to test my setup, at some
points the 1 us I use violates this.

For debug use, we need adapter name from i2c_adapter everywhere.
Current debugging is useless, while the original was simply
incomprehensible. Sorry about the misspelling.

Improved symmetry; outb/inb now both handle the acknowledge timeslot.
Sendbytes/readbytes only loop over the buffer and test for failures.
Pass i2c_msg->flags deeper down to support more i2c mangling.

The i2c_adapter->retry now applies to complete message. Previously
it would fail on any [NA] after the address [A].
I am not 100% sure how auto-incrementing EEPs will tolerate it,
if we fail in the middle of a page and then resend from the beginning.

A message may be sent correctly after ignored [NA], retry or timeout.
The caller could check this in i2c_msg->err, if necessary.


=======================================================================
MDS:

thanks for the notes and the opportunity to make comments.

As I've already said, I like the 50/50 duty cycle, with
1 ns hold and (udelay - 1) setup. That's the way it should be.

One question - is clock low time == udelay at end of a
read, including end of i2c_inb and i2c_stop? it looks to me like
it's only Tmin + Tmin?

One other general comment - sdalo() and sdahi() could be deleted
and inlined into test_bus(), which is the only place they're used?

Other comments below.

Kyosti Malkki wrote:
> I have been reading the "Fast Mode" specs to test my setup, at some
> points the 1 us I use violates this.

Disagree. If you think about it, the max hold spec really only applies
when you are an i2c slave. When you are a master and driving the clock,
you can exceed the max hold time since you are essentially
"extending the clock low time" as described in the footnote.
If 1 us > 0.9 us spec is a problem, then we certainly have a
problem now with a hold time == udelay.

>
> For debug use, we need adapter name from i2c_adapter everywhere.
> Current debugging is useless, while the original was simply
> incomprehensible. Sorry about the misspelling.
>

Agreed that it wasn't very good in the past.

> Improved symmetry; outb/inb now both handle the acknowledge timeslot.
> Sendbytes/readbytes only loop over the buffer and test for failures.
> Pass i2c_msg->flags deeper down to support more i2c mangling.
>

Sounds good.

> The i2c_adapter->retry now applies to complete message. Previously
> it would fail on any [NA] after the address [A].
> I am not 100% sure how auto-incrementing EEPs will tolerate it,
> if we fail in the middle of a page and then resend from the beginning.
>

We may want to think about this more. Or maybe do something different
on reads than on writes. We don't want to corrupt eeproms.

> A message may be sent correctly after ignored [NA], retry or timeout.
> The caller could check this in i2c_msg->err, if necessary.
>

OK. But the whole mechanism of saving and checking the error, etc. is somewhat elaborate,
not sure if useful or not. If you look at the smbus-layer calls (which in turn use
the "emulation layer" if the adapter is i2c-only), errors aren't returned at all.
Which isn't great but that's the way it is now. In fact we have a two-year-old ticket
asking for improvement. I don't have any answers, just wondering if anybody
has a "vision" (!) on error handling and whether the changes in i2c-algo-bit
fit that vision.

=======================================================================
KM:

> One question - is clock low time == udelay at end of a
> read, including end of i2c_inb and i2c_stop? it looks to me like
> it's only Tmin + Tmin?

Did you mean "end of i2c_inb and i2c_outb"? At end of i2c_stop clock
remains high, perhaps need to insert missing "Stop to Start" delay
there.

For repetive inb/outb clock low after ACK is T_hold+T_setup == T_scllo.
Between inb/outb ACK and stop, it is T_hold+T_min == T_min+T_min,
should really be T_hold+T_setup there.

> One other general comment - sdalo() and sdahi() could be deleted
> and inlined into test_bus(), which is the only place they're used?

I thought of adding 'paranoid' mode, that reads back bus state after
any set. This is likely to recombine bus set, get and delay in one
inlined call.

> > I have been reading the "Fast Mode" specs to test my setup, at some
> > points the 1 us I use violates this.
>
> Disagree. If you think about it, the max hold spec really only applies
> when you are an i2c slave.

Actually, I meant 1 us violates "bus free time between Stop and
Start", "Low period of SCL" and "SCL clock frequency".
With 1us resolution best we can do within fast mode specs is
udelay=2 and get 250 kHz. I think stock kernel tree doesn't export
anything to improve the resolution, so I will settle for udelay.

> > The i2c_adapter->retry now applies to complete message. Previously
> > it would fail on any [NA] after the address [A].
> > I am not 100% sure how auto-incrementing EEPs will tolerate it,
> > if we fail in the middle of a page and then resend from the beginning.
>
> We may want to think about this more. Or maybe do something different
> on reads than on writes. We don't want to corrupt eeproms.

Yep. Thinking more into it, correct retry sequence is device specific if
failure happens after address byte. Both i2c<->async and i2c<->parallel
converters should continue with with the first failed byte.

Should we keep adapter->retry for address retry? Getting [NA] for
address may reflect eeprom busy writing, disconnected device or
bus problems. Do SMBus hosts ignore this?

> > A message may be sent correctly after ignored [NA], retry or timeout.
> > The caller could check this in i2c_msg->err, if necessary.
>
> OK. But the whole mechanism of saving and checking the error, etc. is
> somewhat elaborate, not sure if useful or not. If you look at the
> smbus-layer calls (which in turn use the "emulation layer" if the
> adapter is i2c-only), errors aren't returned at all.

With algo-bit this is messy, for SMBus style adapter, you can get return
code from status register directly? For simplicity, the errorcode
set in adapter driver could pass unchanged to the caller. If adapter
drivers already return <0 for failure, nothing is changed from the
caller's point of view. Unless they test for -1 explicitly, which they
should not.

Some means (besides log) to report a stuck or disconnected bus may be
necessary to support removable SMBus devices like batteries, AC adapters
etc.


=======================================================================
KM:

> One question - is clock low time == udelay at end of a
> read, including end of i2c_inb and i2c_stop? it looks to me like
> it's only Tmin + Tmin?

Did you mean "end of i2c_inb and i2c_outb"? At end of i2c_stop clock
remains high, perhaps need to insert missing "Stop to Start" delay
there.

For repetive inb/outb clock low after ACK is T_hold+T_setup == T_scllo.
Between inb/outb ACK and stop, it is T_hold+T_min == T_min+T_min,
should really be T_hold+T_setup there.

> One other general comment - sdalo() and sdahi() could be deleted
> and inlined into test_bus(), which is the only place they're used?

I thought of adding 'paranoid' mode, that reads back bus state after
any set. This is likely to recombine bus set, get and delay in one
inlined call.

> > I have been reading the "Fast Mode" specs to test my setup, at some

> BTW, I want to make sure you've read the suggestions Dori Eldar made
> this summer, which I summarized in i2c/TODO. They're the ones marked
> "D.E.".

Yes. I got some idea what is doable and what to drop on the floor.

> > Actually, I meant 1 us violates "bus free time between Stop and
> > Start", "Low period of SCL" and "SCL clock frequency".
> > With 1us resolution best we can do within fast mode specs is
> > udelay=2 and get 250 kHz. I think stock kernel tree doesn't export
> > anything to improve the resolution, so I will settle for udelay.
> >
> Agreed. Fast mode max is a udelay of 1.25. If people want to
> violate that and set a udelay of 1, they can at their peril.
> We should add documentation that a udelay of 2 (250 kHz) is a practical
> lower limit and we do not recommend a udelay of 1.
>
> When nanosleep or whatever gets exported, we can do better.

I decided to go for a new algorithm driver that pushes bus timing to the
adapter code. Currently it falls back to udelay(), but could use any
approriate counter/timer available.
I hope to get this tested a bit and commited today.

> Reading ticket 517 more carefully, the i2c-core layer DOES return
> error codes pretty faithfully. I thing the bus drivers do too.
> So I misspoke. It's the CHIP drivers that generally ignore the
> error returns and the -1 return gets cast to an FF value.

Yes, i2c-core gives -1 for any error. Being more specific about what
went wrong can be done without breaking compatibility. In i2c_transfer,
we could pass return from algo->xfer as-is.

> Should we keep adapter->retry? Seems like more trouble than it is worth.
> Maybe you could hold it back as part 2 of the patch?

Well my patch got so radical that I broke compatibility, and some
changes are not easy to port back.
I think I have caught and fixed following problems in original code:

All the duty cycle stuff.

If SDA is stuck low, writes seem to work, reads returns 0x00.
If SCL is stuck low, we keep hitting timeout.
The driver should die more gracefully on bus failure, and advice the
caller to give up by returning -ENODEV or something besides -1.

After SCL timeout, SCL is not pulled low by host.
Client may release SCL at any time between timeout and stop.
At some situations, timeout is ignored and/or SDA is toggled.
Stop assumes SCL low on entry.


> The i2c_smbus_xxx_xxx calls return an s32, -1 on failure.
> In a chip driver, for example, it should probably check for -1
> returns and throw out that reading, and show the user the last good rea=
ding.
> Otherwise, cast s32 to u8 as usual.

Why test for -1 explicitly, just test negative for error?


=======================================================================
KM:

Here we go. In order of the TODO.

1. Timing considerations

Maximum of 50us SCL high, I quess this is an alternative for detecting
bus busy condition instead of Start-to-Stop. Sampling for 50us before
start is easier to achieve than sampling lines all the time for
Start/Stop. Neither was done by i2c-algo-bit, nor I have plans to do so.

With new driver these can be supported, if approriate capture/compare
unit is wired on the SCL line. In this case adapter code does not use
the provided inlines from i2c-algo-biths.h.


2. Arbitration

Without busy detection, no point for arbitration. The best we can do is
stop the message that already was corrupted if we notice difference in
set and get bus state. It is easy to change the recovery action from
Stop to release, if bus busy is detected.


Both issues can be ignored on a single-master system. One designing
multi-master bus system, really should use HW glue logic for SMBus
access anyway.