[ECOS] Request for comments onCYG_I2C_BITBANG_SCL_LOW_SDA_INPUT patch

Tue Mar 7 10:30:00 GMT 2006

>>>>>> "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.
>
>    Oyvind> My I2C + hardware skills are fairly shaky, so any and all
>    Oyvind> comments are most welcome!
>
> Looking at the patch to the bit-banging code:
>
> Index: src/i2c.cxx
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/io/i2c/current/src/i2c.cxx,v
> retrieving revision 1.3
> diff -u -r1.3 i2c.cxx
> --- src/i2c.cxx 15 Jul 2005 13:52:27 -0000 1.3
> +++ src/i2c.cxx 6 Mar 2006 21:38:40 -0000
> @@ -296,6 +296,10 @@
(*)>         (*banger)(mash, (0 != mask) ? CYG_I2C_BITBANG_SCL_LOW : 
CYG_I2C_BITBANG_SCL_LOW_SDA_INPUT);
>         HAL_DELAY_US(delay);
>     }
> +    // SCL is stable low, we can now drop SDA
> +    (*banger)(mash, CYG_I2C_BITBANG_SDA_HIGH);
> +    HAL_DELAY_US(delay);
> +
>     // The last bit has been sent, SCL is low, and SDA has been turned
>     // into an input. The device should be placing the acknowledge bit
>     // onto SDA. Reading the acknowledge bit still needs a clock cycle.
>
> This makes no sense. Assuming typical hardware where a single GPIO pin
> is used for SDA and another for SCL, at the end of the loop the SDA
> pin will have been turned into an input. Setting it as an output-high
> will only change a bit in some GPIO control register, it will have no
> effect on what is happening on the wire.

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

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

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

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

Conclusion:
On Oyvind's code changes, replacing (*) with
 (*banger)(mash, CYG_I2C_BITBANG_SCL_LOW);
would be the best and only safe solution where you use proper type 1 I/O.

> @@ -356,6 +360,9 @@
>     HAL_DELAY_US(delay);
(*)>     (*banger)(mash, nak ? CYG_I2C_BITBANG_SCL_LOW : 
CYG_I2C_BITBANG_SCL_LOW_SDA_INPUT);
>     HAL_DELAY_US(delay);
> +    // SCL is stable low, we can now drop SDA
> +    (*banger)(mash, CYG_I2C_BITBANG_SDA_HIGH);
> +    HAL_DELAY_US(delay);
>
>     DEBUG("i2c bitbang read_byte, result %02x\n", result);
>     return result;
>
> Unless the nak flag is set, the SDA GPIO pin will again be an input so
> setting it output-high has no effect on the wires. If the nak flag is
> set, i.e. if we are at the end of a receive, then we are equally
> likely to want SDA high for a subsequent repeated start or low for a
> stop. I see little point in forcing it high.

Same as above.. Changing SDA before SCL has stabelized to low, may cause 
problems.

> On the more general issue, if I understand correctly you are concerned
> about what is happening on the SDA line during an SCL_LOW_SDA_INPUT
> operation. If the SDA line was already high then it won't change, the
> pull-up resistor will keep it high, so we are only worried about SDA
> transitions from low to high.

True.

> SCL dropping from high to low involves discharging the line's
> capacitance through a low resistance. Further, the banger function
> should perform this step first: the operation is SCL_LOW_SDA_INPUT,
> not SDA_INPUT_SCL_LOW. SDA going low to high involves charging the
> line's capacitance through a pull-up resistor. That should take rather
> longer. On typical hardware I would expect SCL to have dropped to a
> suitably low value well before any significant changes have happened
> on the SDA line. There could be platform-specific problems where the
> SCL line has a much higher capacitance than SDA, but those should be
> addressed by some dummy cycles within the bit-bang function.

Yes, the order of SCL_LOW and SDA_INPUT  is an important point. However I 
would not trust your assumptions about "typical" hardware connected to the 
bus. I2C may run over long cables (DDC) and have different complicated 
loads. I would like to see the drivers stable on all solutions, where only 
the bitrate should be adjusted to compensate for wiring complexity.

> I assume these patches were the result of trying to get the
> bit-banging code running on some real hardware. What is the hardware?

I work on the same project as Oyvind. The hardware is FPGA I/O's  type 1 
where there are several I2C bus'es, some onboardwith known loads, some 
external where the load is unknown (both electrically, i2c implementation 
type and wires), and where there is hot-plugging (DDC, external I/O 
devices). The potential problem was actually discovered when implementing 
SCL_LOW_SDA_INPUT in the wrong direction. I do not feel it's a safe solution 
doing SCL_LOW then SDA_INPUT with an undefined delay in between.

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

Thanks for the attention,
Morten Leikvoll

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