root/i2c/trunk/TODO @ 5363

Revision 5363, 24.0 KB (checked in by khali, 6 years ago)

Drop I2C_FUNC_SMBUS_*I2C_BLOCK_2 defines. They never went in kernel 2.4
and were just dropped from kernel 2.6.

  • Property svn:eol-style set to native
  • Property svn:keywords set to Author Date Id Revision
Line 
1I2C / SMBus TODO list
2Contact us if you have comments or wish to help.
3------------------------------------------------
4
5* Only chip driver modules are locked in memory when one accesses their
6  /proc entries. Bus drivers are never locked (i.e. no reference count
7  is done). Bus drivers should be locked by the simple fact that a client
8  uses them (which would make it impossible to unload them before
9  all using client drivers have been unloaded themselves). This might be
10  restricted to clients that actually have /proc entries.
11
12* "uninstall" Makefile target.
13
14* i2c-algo-bit problems, and unfinished i2c-algo-biths
15  See the following two entries marked D.E. (Dori Eldar's suggestions),
16  and see the email thread at the bottom of this file.
17
18* Timing considerations in SMBus emulation with i2c-algo-bit (D.E.):
19  (Note that some of these changes are implemented in i2c-algo-biths)
20  The SMBus defines a minimum frequency of 10 kHz for driving the bus, while
21  the I2C does not define any minimum frequency.
22  furthermore the maximum time a master is allowed to keep the CLK line high
23  is 50 usec. this is required so masters can detect an idle bus
24  (the CLK line is high for at least 50 usec), again the I2C does not impose
25  this restriction.
26  It's crucial that masters will obey the last timing consideration, since:
27        1. Slaves may otherwise hang the transaction.
28        2. Other masters will assume the bus is idle, initiate their own SMBus
29           transaction, which will lead to corruption of data
30           carried over the SMBus.
31  Note that a correct arbitration procedure can take
32  place only if masters are "synchronized" meaning, they initiate the
33  transaction at the _same_ time.
34
35  Now when implementing the SMBus protocol in SW one has to make sure that in
36  the critical sections in which the CLK is held high, SW is not preempted.
37  Or more simply: SW must disable interrupts at this period of time.
38  This critical periods are:
39        a. Waiting for an idle bus before starting the transaction until the
40           CLK is driven low after generating the START signal.
41        b. For each clock cycle in the transaction: before the CLK is set
42           to HIGH till after the CLK is set LOW.
43
44  Looking at the i2c-algo-bit.c it's very problematic to add the interrupt
45  disabling code since the code may sleep in the critical period at the
46  "sclhi" function.
47  You would also need to add a "wait for idle bus" before the driving the
48  START signal.
49
50* Arbitration in SMBus emulation with i2c-algo-bit (D.E.):
51  Although the I2C disallows arbitration between masters while one is sending
52  a restart signal & another sending a data signal, this situation is
53  theoretically possible on an SMBus.
54  So you would need to check for arbitration when driving the restart signal
55  too. BTW why is the arbitration check code disabled in the i2c_outb?
56
57* Pre-Post routines with i2c-algo-bit (D.E.):
58  Before & after driving an Smbus transaction, if HW must be accessed, such
59  access is not possible with current algo-bit design. It's possible to allow
60  such access by doing
61  an EXPORT_SYMBOL(i2c_bit_xfer) therefore making the algo-bit available as a
62  library function only. Or by adding a post & pre function pointers to the
63  algo-bit structure.
64
65* 16-bit Register Addresses
66  There is no support for 16-bit register addresses (used by serial
67  eeproms larger than 16K bit, such as the Atmel 24C32) in i2c-core.c
68  or i2c-dev.c.
69  Emulation support is required.
70  General 16-bit support for all transaction types will require
71  many changes.
72  Note that writes with an arbitrary number of address bytes
73  are actually supported now by treating the extra bytes as
74  the beginning of the data.
75  There is an alternate proposal from Wolfgang Denk <wd@denx.de>
76  for a driver 'i2c-simple' that supports a generalized address
77  length of 1-4 bytes and accesses via ioctls on /dev.
78
79* Host-as-slave mode in i2c adapters may need to be integrated
80  with the adapter host code, because the lm_sensors-type chip driver
81  architecture is not well suited to implementing a chip slave,
82  the slave mode will require locking with the master mode, and
83  specialized commmunication such as "Notify ARP Master".
84  Adapters may respond to one or more host-as-slave addresses. The
85  functionality bits and the rest of the API will have to be extended
86  to support slaves embedded in host adapters.
87
88* 64-bit functionality values may be required to represent all
89  the new capabilities described above.
90
91* The Maximum SMBus block transfer size is 32 bytes,
92  as defined in the SMBus specification, and
93  assumption is scattered throughout the code. Replace hardcoded '32'
94  throughout the code with I2C_SMBUS_BLOCK_MAX to be clear.
95
96* Emulation layer i2c block reads are fixed at 32 bytes and there is
97  currently no method to change it. You cannot, for example,
98  read an entire serial eeprom with a single block read, or read
99  only the 7 bytes in a clock chip. There is no i2c limitation on
100  block size but the code is designed for the 32 byte limit of
101  SMBus.
102
103* i2c version strings were added to i2c.h but they are used only
104  for printk's. Integers would be better for use in preprocessor
105  directives for conditional compiles.
106
107* Alternative i2c implementations in kernel to be converted to
108  the standard i2c implementation in this package.
109  Most if not all of these are bit-banging algorithms,
110  for which the official driver is drivers/i2c/i2c-algo-bit.c.
111  For a good example of using i2c-algo-bit, see drivers/acorn/char/i2c.c.
112  i2c-old.h was removed in kernel 2.5.1.
113
114        drivers/media/video/i2c-old.c   Used by:
115                drivers/media/video/buz.c
116                drivers/media/video/i2c-parport.c
117                drivers/media/video/saa7110.c
118                drivers/media/video/saa7111.c
119                drivers/media/video/saa7185.c
120                drivers/media/video/stradis.c
121                drivers/media/video/zr36120.c
122                drivers/media/video/zr36120_i2c.c
123        arch/ppc/mbxboot/iic.c
124        drivers/media/radio/radio-trust.c
125        drivers/media/video/pms.c
126        drivers/media/video/stradis.c
127        drivers/net/acenic.c
128        drivers/net/sk98lin/ski2c.c
129        drivers/sbus/char/envctrl.c
130        drivers/sbus/char/vfc_i2c.c
131        drivers/scsi/cpqfcTSi2c.c
132        drivers/usb/ov511.c
133
134* Make especially i2c-core SMP-safe. This means: locks around all global
135  variable access, especially during (de)registration activity.
136
137* Check debugging levels in all modules are sane.
138
139* linux/Documentation/devices.txt lists i2c0, i2c1 etc. instead of i2c-0, i2c-1
140
141* At least the bit-lp and bit-velle modules do no detection on loading;
142  ask Simon whether this is possible to add.
143
144* Correct all module locking code (see Keith Owens' email about this)
145
146* i2c-algo-bit problems, and unfinished i2c-algo-biths
147  Below is a reformatted email thread between Kyosti Malkki and MDS,
148  November-December 2002, on the lm_sensors mailing list.
149  This discussion identifies timing and multi-master
150  problems in i2c-algo-bit. Kyosti wrote a new driver, i2c-algo-biths,
151  which attempts to fix these problems. The new driver has not been
152  heavily tested and may be incomplete; the old driver still has
153  the problems identified below:
154
155=======================================================================
156Kyosti Malkki wrote:
157
158>
159> I have no scope around to check this, but looking into i2c-algo-bit.c,
160> adap->udelay is used where it is not necessary. As an example, udelay=3
161> would look roughly like 25/75 duty cycle:
162>
163> SDA: -._________---._________---.------------.-
164> SCL: _.___---______.___---______.___---______.-
165>         ^     ^  ^   ^     ^  ^   ^     ^  ^
166>
167> Dots just point out where sw goes for next bit, others 1 us wide each.
168> That is, all bus transitions use adap->udelay, where a rise/fall delay
169> of 1 us (or less) would do.
170>
171> Removing the unnecessary 2us marked ^ _above_ :
172>
173> SDA: -._____-._____-.------.-
174> SCL: _._---__._---__._---__.-
175>             ^ ^      ^    ^
176>
177> When successive bits to put on bus are equal, SDA rise/fall may be
178> omitted.
179>
180> SDA: -._____.____-.-----.-
181> SCL: _._---_.---__.---__.-
182>
183> Interrupt service and PCI latency may add to the delay time.  Any
184> decrease is not possible, if write-read sequence is used in
185> setscl() and setsda(). Right ?
186>
187> In the process I tested using nominal 1us delay everywhere, and
188> i2c_adap->udelay only to clock bits in and out.  I believe it does not
189> violate specs, but will likely fail with higher bus capacitance.
190> This changes duty-cycle and might break some support, so I won't commit
191> it now. It does improve the speed upto 333 kbps, using adap->udelay=1.
192>
193
194=======================================================================
195MDS:
196
197The last time I used a scope on the i2c bus (quite a while ago), I was
198surprised to see that
199the clock was not a 50/50 duty cycle. But that's what the code does.
200I don't think it's correct, or at least it isn't optimal.
201
202If I'm looking at your 2nd picture correctly, it looks like it
203implements
204a 50/50 duty cycle. That looks right to me.
205I don't understand why there are udelays() in the sdalo() and sdahi()
206functions.
207
208I just recently added some comments to i2c-algo-bit.h about the units,
209(I kept forgetting myself, and others would ask periodically too)
210and made some fixes to the drivers to handle HZ != 100.
211
212What the comment I added says, and it isn't strictly true,
213is that clock rate = 500,000 / udelay. It does mean that
214the minimum half-cycle time (either high _or_ low) is == udelay.
215
216Chip datasheets generally specify maximum I2C clock rate.
217They _dont_ specify minimums for clock low or clock high,
218it is implied to be == 1 / (2 *  maximum rate).
219
220We don't want to violate that. If udelay == 5,
221(which implies a 100 kHz clock) that should
222be the minimum time high _or_ low for the clock.
223So I don't think your third picture below should be implemented.
224
225I agree that any latency assurance should be in the setsda() and
226setscl() functions.
227
228=======================================================================
229KM:
230
231On Wed, 27 Nov 2002, Mark D. Studebaker wrote:
232
233> If I'm looking at your 2nd picture correctly, it looks like it
234> implements a 50/50 duty cycle. That looks right to me. I don't
235> understand why there are udelays() in the sdalo() and sdahi()
236> functions.
237
238It was udelay/3 duty cycle. Some setup and hold time is required,
2391 us is around 66 PCI clocks.
240
241> We don't want to violate that. If udelay == 5,
242> (which implies a 100 kHz clock) that should
243> be the minimum time high _or_ low for the clock.
244
245If we use 1us in sdahi/lo, and udelay=3 for sclhi/lo,
246we get duty cycle of udelay/(udelay+2):
247
248SDA: -__--------_______---------
249SCL: --__---_____---_____---____
250
251Finally, with SDA half of SCL clock rate and (udelay-1) offset:
252
253SDA: -______________-------------______
254SCL: --___---___---___---___---___---___
255
256As for other issues in algo-bit:
257
258At several locations, return code of sclhi() is silently ignored,
259
260I think I replace i2c_start with i2c_repstart -- if SCL is already low,
261we may have multi-master or bus stuck. To count for possibility of a
262second master, we should read back SDA level on master write slots.
263
264Are return values of i2c_transfer and friends explained somewhere?
265
266
267=======================================================================
268MDS:
269
270I looked carefully at the loops in i2c_outb() and i2c_inb().
271These are what set the timing and duty cycle for most of an i2c bus
272cycle.
273I'm (for the moment) ignoring timing and issues outside these loops.
274If any of these statements don't look correct, speak up :)
275
276- i2c_outb generates a 33% duty cycle clock (udelay high, 2 * udelay
277low)
278- i2c_inb generates a 50/50 clock (udelay high, udelay low)
279- Neither function calls sdahi or sdalo inside its loop in normal
280operation,
281   so changes to sdahi/sdalo won't affect the duty cycle.
282- It appears that we can make changes to i2c_outb to get a 50/50 clock.
283  The loop from i2c_outb() is annotated below. Note that these
284  changes don't correctly handle delays coming into and out of the loop.
285
286
287
288        /* assert: scl is low */
289        for ( i=7 ; i>=0 ; i-- ) {
290                sb = c & ( 1 << i );
291                setsda(adap,sb);
292// The following udelay ensures setup time to the r.e. of the clock,
293// and ensures the clock low time (together with the udelay at the
294bottom of the loop)
295// It could be changed to udelay(adap->udelay - 1)
296                udelay(adap->udelay);
297                DEBPROTO(printk(KERN_DEBUG "%d",sb!=0));
298// The following call to sclhi() contains a call to delay(adap->udelay)
299// which ensures the clock high time
300                if (sclhi(adap)<0) { /* timed out */
301                        sdahi(adap); /* we don't want to block the net */
302                        DEB2(printk(KERN_DEBUG " i2c_outb: 0x%02x, timeout at bi                       
303                        return -ETIMEDOUT;
304                };
305                /* do arbitration here:
306                 * if ( sb && ! getsda(adap) ) -> ouch! Get out of here.
307                 */
308                setscl(adap, 0 );
309// The following udelay ensures hold time after the f.e. of the clock,
310// and ensures the clock low time (together with the udelay at the top
311of the loop)
312// It could be changed to udelay(1) and still meet the 300 ns hold time
313// (ref. I2C bus spec version 2.1, table 5, note 2)
314                udelay(adap->udelay);
315        }
316
317=======================================================================
318KM:
319
320Unfortunately sclhi() does not ensure clock high time the right way.
321The delay should be after we read clock as high.
322
323Usually this means we get no acknowledge from the client, since it has
324missed this one clock cycle. If we miss a bit during the address bits,
325we may access incorrect chips or more than one chip at a time.
326
327
328=======================================================================
329KM:
330(comments about his i2c-algo-biths patch)
331
332Some notes to comment on the changes in the patch..
333
334I have been reading the "Fast Mode" specs to test my setup, at some
335points the 1 us I use violates this.
336
337For debug use, we need adapter name from i2c_adapter everywhere.
338Current debugging is useless, while the original was simply
339incomprehensible. Sorry about the misspelling.
340
341Improved symmetry; outb/inb now both handle the acknowledge timeslot.
342Sendbytes/readbytes only loop over the buffer and test for failures.
343Pass i2c_msg->flags deeper down to support more i2c mangling.
344
345The i2c_adapter->retry now applies to complete message. Previously
346it would fail on any [NA] after the address [A].
347I am not 100% sure how auto-incrementing EEPs will tolerate it,
348if we fail in the middle of a page and then resend from the beginning.
349
350A message may be sent correctly after ignored [NA], retry or timeout.
351The caller could check this in i2c_msg->err, if necessary.
352
353
354=======================================================================
355MDS:
356
357thanks for the notes and the opportunity to make comments.
358
359As I've already said, I like the 50/50 duty cycle, with
3601 ns hold and (udelay - 1) setup. That's the way it should be.
361
362One question - is clock low time == udelay at end of a
363read, including end of i2c_inb and i2c_stop? it looks to me like
364it's only Tmin + Tmin?
365
366One other general comment - sdalo() and sdahi() could be deleted
367and inlined into test_bus(), which is the only place they're used?
368
369Other comments below.
370
371Kyosti Malkki wrote:
372> I have been reading the "Fast Mode" specs to test my setup, at some
373> points the 1 us I use violates this.
374
375Disagree. If you think about it, the max hold spec really only applies
376when you are an i2c slave. When you are a master and driving the clock,
377you can exceed the max hold time since you are essentially
378"extending the clock low time" as described in the footnote.
379If 1 us > 0.9 us spec is a problem, then we certainly have a
380problem now with a hold time == udelay.
381
382>
383> For debug use, we need adapter name from i2c_adapter everywhere.
384> Current debugging is useless, while the original was simply
385> incomprehensible. Sorry about the misspelling.
386>
387
388Agreed that it wasn't very good in the past.
389
390> Improved symmetry; outb/inb now both handle the acknowledge timeslot.
391> Sendbytes/readbytes only loop over the buffer and test for failures.
392> Pass i2c_msg->flags deeper down to support more i2c mangling.
393>
394
395Sounds good.
396
397> The i2c_adapter->retry now applies to complete message. Previously
398> it would fail on any [NA] after the address [A].
399> I am not 100% sure how auto-incrementing EEPs will tolerate it,
400> if we fail in the middle of a page and then resend from the beginning.
401>
402
403We may want to think about this more. Or maybe do something different
404on reads than on writes. We don't want to corrupt eeproms.
405
406> A message may be sent correctly after ignored [NA], retry or timeout.
407> The caller could check this in i2c_msg->err, if necessary.
408>
409
410OK. But the whole mechanism of saving and checking the error, etc. is somewhat elaborate,
411not sure if useful or not. If you look at the smbus-layer calls (which in turn use
412the "emulation layer" if the adapter is i2c-only), errors aren't returned at all.
413Which isn't great but that's the way it is now. In fact we have a two-year-old ticket
414asking for improvement. I don't have any answers, just wondering if anybody
415has a "vision" (!) on error handling and whether the changes in i2c-algo-bit
416fit that vision.
417
418=======================================================================
419KM:
420
421> One question - is clock low time == udelay at end of a
422> read, including end of i2c_inb and i2c_stop? it looks to me like
423> it's only Tmin + Tmin?
424
425Did you mean "end of i2c_inb and i2c_outb"? At end of i2c_stop clock
426remains high, perhaps need to insert missing "Stop to Start" delay
427there.
428
429For repetive inb/outb clock low after ACK is T_hold+T_setup == T_scllo.
430Between inb/outb ACK and stop, it is T_hold+T_min == T_min+T_min,
431should really be T_hold+T_setup there.
432
433> One other general comment - sdalo() and sdahi() could be deleted
434> and inlined into test_bus(), which is the only place they're used?
435
436I thought of adding 'paranoid' mode, that reads back bus state after
437any set. This is likely to recombine bus set, get and delay in one
438inlined call.
439
440> > I have been reading the "Fast Mode" specs to test my setup, at some
441> > points the 1 us I use violates this.
442>
443> Disagree. If you think about it, the max hold spec really only applies
444> when you are an i2c slave.
445
446Actually, I meant 1 us violates "bus free time between Stop and
447Start", "Low period of SCL" and "SCL clock frequency".
448With 1us resolution best we can do within fast mode specs is
449udelay=2 and get 250 kHz. I think stock kernel tree doesn't export
450anything to improve the resolution, so I will settle for udelay.
451
452> > The i2c_adapter->retry now applies to complete message. Previously
453> > it would fail on any [NA] after the address [A].
454> > I am not 100% sure how auto-incrementing EEPs will tolerate it,
455> > if we fail in the middle of a page and then resend from the beginning.
456>
457> We may want to think about this more. Or maybe do something different
458> on reads than on writes. We don't want to corrupt eeproms.
459
460Yep. Thinking more into it, correct retry sequence is device specific if
461failure happens after address byte. Both i2c<->async and i2c<->parallel
462converters should continue with with the first failed byte.
463
464Should we keep adapter->retry for address retry? Getting [NA] for
465address may reflect eeprom busy writing, disconnected device or
466bus problems. Do SMBus hosts ignore this?
467
468> > A message may be sent correctly after ignored [NA], retry or timeout.
469> > The caller could check this in i2c_msg->err, if necessary.
470>
471> OK. But the whole mechanism of saving and checking the error, etc. is
472> somewhat elaborate, not sure if useful or not. If you look at the
473> smbus-layer calls (which in turn use the "emulation layer" if the
474> adapter is i2c-only), errors aren't returned at all.
475
476With algo-bit this is messy, for SMBus style adapter, you can get return
477code from status register directly? For simplicity, the errorcode
478set in adapter driver could pass unchanged to the caller. If adapter
479drivers already return <0 for failure, nothing is changed from the
480caller's point of view. Unless they test for -1 explicitly, which they
481should not.
482
483Some means (besides log) to report a stuck or disconnected bus may be
484necessary to support removable SMBus devices like batteries, AC adapters
485etc.
486
487
488=======================================================================
489KM:
490
491> One question - is clock low time == udelay at end of a
492> read, including end of i2c_inb and i2c_stop? it looks to me like
493> it's only Tmin + Tmin?
494
495Did you mean "end of i2c_inb and i2c_outb"? At end of i2c_stop clock
496remains high, perhaps need to insert missing "Stop to Start" delay
497there.
498
499For repetive inb/outb clock low after ACK is T_hold+T_setup == T_scllo.
500Between inb/outb ACK and stop, it is T_hold+T_min == T_min+T_min,
501should really be T_hold+T_setup there.
502
503> One other general comment - sdalo() and sdahi() could be deleted
504> and inlined into test_bus(), which is the only place they're used?
505
506I thought of adding 'paranoid' mode, that reads back bus state after
507any set. This is likely to recombine bus set, get and delay in one
508inlined call.
509
510> > I have been reading the "Fast Mode" specs to test my setup, at some
511
512> BTW, I want to make sure you've read the suggestions Dori Eldar made
513> this summer, which I summarized in i2c/TODO. They're the ones marked
514> "D.E.".
515
516Yes. I got some idea what is doable and what to drop on the floor.
517
518> > Actually, I meant 1 us violates "bus free time between Stop and
519> > Start", "Low period of SCL" and "SCL clock frequency".
520> > With 1us resolution best we can do within fast mode specs is
521> > udelay=2 and get 250 kHz. I think stock kernel tree doesn't export
522> > anything to improve the resolution, so I will settle for udelay.
523> >
524> Agreed. Fast mode max is a udelay of 1.25. If people want to
525> violate that and set a udelay of 1, they can at their peril.
526> We should add documentation that a udelay of 2 (250 kHz) is a practical
527> lower limit and we do not recommend a udelay of 1.
528>
529> When nanosleep or whatever gets exported, we can do better.
530
531I decided to go for a new algorithm driver that pushes bus timing to the
532adapter code. Currently it falls back to udelay(), but could use any
533approriate counter/timer available.
534I hope to get this tested a bit and commited today.
535
536> Reading ticket 517 more carefully, the i2c-core layer DOES return
537> error codes pretty faithfully. I thing the bus drivers do too.
538> So I misspoke. It's the CHIP drivers that generally ignore the
539> error returns and the -1 return gets cast to an FF value.
540
541Yes, i2c-core gives -1 for any error. Being more specific about what
542went wrong can be done without breaking compatibility. In i2c_transfer,
543we could pass return from algo->xfer as-is.
544
545> Should we keep adapter->retry? Seems like more trouble than it is worth.
546> Maybe you could hold it back as part 2 of the patch?
547
548Well my patch got so radical that I broke compatibility, and some
549changes are not easy to port back.
550I think I have caught and fixed following problems in original code:
551
552All the duty cycle stuff.
553
554If SDA is stuck low, writes seem to work, reads returns 0x00.
555If SCL is stuck low, we keep hitting timeout.
556The driver should die more gracefully on bus failure, and advice the
557caller to give up by returning -ENODEV or something besides -1.
558
559After SCL timeout, SCL is not pulled low by host.
560Client may release SCL at any time between timeout and stop.
561At some situations, timeout is ignored and/or SDA is toggled.
562Stop assumes SCL low on entry.
563
564
565> The i2c_smbus_xxx_xxx calls return an s32, -1 on failure.
566> In a chip driver, for example, it should probably check for -1
567> returns and throw out that reading, and show the user the last good rea=
568ding.
569> Otherwise, cast s32 to u8 as usual.
570
571Why test for -1 explicitly, just test negative for error?
572
573
574=======================================================================
575KM:
576
577Here we go. In order of the TODO.
578
5791. Timing considerations
580
581Maximum of 50us SCL high, I quess this is an alternative for detecting
582bus busy condition instead of Start-to-Stop. Sampling for 50us before
583start is easier to achieve than sampling lines all the time for
584Start/Stop. Neither was done by i2c-algo-bit, nor I have plans to do so.
585
586With new driver these can be supported, if approriate capture/compare
587unit is wired on the SCL line. In this case adapter code does not use
588the provided inlines from i2c-algo-biths.h.
589
590
5912. Arbitration
592
593Without busy detection, no point for arbitration. The best we can do is
594stop the message that already was corrupted if we notice difference in
595set and get bus state. It is easy to change the recovery action from
596Stop to release, if bus busy is detected.
597
598
599Both issues can be ignored on a single-master system. One designing
600multi-master bus system, really should use HW glue logic for SMBus
601access anyway.
602
603
Note: See TracBrowser for help on using the browser.