[ECOS] Request for comments onCYG_I2C_BITBANG_SCL_LOW_SDA_INPUT patch

[Bart Veer]

>>>>> "Morten" == Morten Leikvoll <leikvoll@cyviz.com> writes:

    >>>>>>> "Oyvind" == =?ISO-8859-1?Q?=D8yvind?= Harboe <ISO-8859-1>
    >>>>>>> writes:

    Oyvind> I've assembled a patch for a race condition where SDA can
    Oyvind> be changed before SCL is stable low.

    <snip>

    Morten> There are different types of IO that are used for I2C, one
    Morten> correct, and several bad ones. Like these:
    Morten> 1) GPIO (the correct way) where you just pull the SDA/SCL
    Morten> lines low by permanently setting the output data to low,
    Morten> and use the direction of the I/O for signaling
    Morten> (in=high/out=low). These signals need to be pulled logic
    Morten> high by resistors.
    Morten> 2) GPIO where you for some reason need to keep SDA driving
    Morten> high for some period, but you do have control of direcion
    Morten> (maybe for electronics where the pullup is missing)
    Morten> 3) 2 I/O lines where one pin is permanently output (with
    Morten> serial resistor) and another is permanently input wich is
    Morten> permanently connected together.

(1) is appropriate for much sensible hardware, but not all. For
example the AT91RM9200 has very flexible GPIO support where you can
set up each pin to work in open collector mode complete with built-in
pull-up resistor, or in push-pull mode. There is no need to switch the
pin between input and output mode, you can read the current state
while driving the pin.

(2) may be appropriate for certain types of less-well designed
hardware, e.g. where the pull-up resistance is far too high. Driving
the pin in push-pull mode may allow for better performance, albeit
with a possible conflict when switching between tx and rx.

Of course (2) should never be necessary, hardware should always be
designed sensibly. However the current bit-banging code was intended
to work on somewhat broken hardware as well. The platform-specific
banger function is given operations such as SCL_LOW_SDA_INPUT rather
than just SCL_LOW, so that it can implement these in a way appropriate
for the target hardware.

    Morten> SCL_LOW_SDA_INPUT seems to be implemented for solution 2
    Morten> and 3, cause setting SCL low and SDA high simultaneously
    Morten> (or near simultaneously) introduces 2 problems:

    Morten> A) Race condition. According to the i2c specs, all devices
    Morten> must see a stable clock low before SDA changes. Depending
    Morten> on the wiring distance, wiring load and CPU speed, this
    Morten> may end up in a slave detecting start/stop conditions.
    Morten> Replacing SCL_LOW_SDA_INPUT by CYG_I2C_BITBANG_SCL_LOW,
    Morten> introducing a delay before releasing SDA will resolve this
    Morten> problem. The discussed changes will do this, provided
    Morten> SCL_LOW_SDA_INPUT is implemented with fallback to SCL_LOW.
    Morten> Ideally SCL_LOW_SDA_INPUT should be removed but this is
    Morten> maybe the best of all evil solution for type 2&3 I/O.

    Morten> B) for a short period of time, the master may drive SDA
    Morten> high, while a slave is driving SDA low. Again,
    Morten> SCL_LOW_SDA_INPUT seems to be the best of all evil
    Morten> solution to prevent this case when using type 2&3 I/O.

Replacing SCL_LOW_SDA_INPUT with SCL_LOW followed by SDA_INPUT is not
safe. The processor may be interrupted between SCL_LOW and SDA_INPUT,
so there could be a long delay between these two operations and the
I2C device may start driving the bus at that time. Instead
SCL_LOW_SDA_INPUT should be an atomic operation, with the banger
function potentially having to disable interrupts for a few cycles. Of
course a conflict can only happen on poorly designed hardware, and on
sensible hardware there is no need to disable interrupts.

If the banger function implements SCL_LOW_SDA_INPUT too fast so that
SDA starts changing before SCL has settled, there is a simple
solution: add an appropriate number of nops to the banger function.
This can and should be done on a platform-specific basis, i.e. in the
banger function. Much hardware will work fine without such a delay,
and it is inappropriate to penalize all platforms with extra delays.

There is probably scope for improving the documentation for the
bit-banging code, especially more information on how the banger
function should work in various circumstances. I'll take another look
at the docs when I get a chance. However I don't see any need to
change the generic bit-banging code for this issue, any problems
should be addressed at the platform level.

Bart

-- 
Bart Veer                       eCos Configuration Architect
http://www.ecoscentric.com/     The eCos and RedBoot experts


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss