i2c: set hold time of SDA during transmit to an appropriate value

[fivdi]

Fixes adafruit/circuitpython#4082

Increases the set hold time of SDA during transmit from its default value of 1 to 2 enabling pico-sdk to function correctly with the TCS34725 color sensor.

On page 48 of the I2C-bus specification and user manual Table 10 lists the "Characteristics of the SDA and SCL bus lines for Standard, Fast, and Fast-mode Plus I2C-bus devices". Below this table note [3] mentions the following:

[3] A device must internally provide a hold time of at least 300 ns for the SDA signal (with respect to the V IH(min) of the SCL signal) to bridge the undefined region of the falling edge of SCL.

This PR sets the IC_SDA_TX_HOLD field in the IC_SDA_HOLD register to value required for a hold time of 300 ns in Standard-mode and Fast-mode and 120 ns in Fast-mode Plus.

The following tests were performed with this PR at 10000, 100000 and 400000 baud with two devices (MCP9808 and TCS34725) on the I2C bus and all tests ran successfully.

• Scan the I2C bus with bus_scan from pico-examples. Without this this PR the TCS34725 is not detected. With this PR the TCS34725 is detected.
• Read the Device ID register form the TCS34725. Without this PR there is an error. With this PR the Device ID register can be successfully read and is 0x44.
• Read the temperature from the MCP9808 temperature sensor in an infinite loop (works with or without this PR.)

Tests were also successfully performed with a BME280 and TCS34725 at 1000000 baud.

Unfortunately, I can't really explain why the modification made by this PR is needed to to get pico-sdk to function correctly with the TCS34725. According to figure 11 on page 10 of the TCS34725 datasheet the minimum data hold time, t(HDDAT), is 0μs and the maximum is 0.9μs. However, the notes below figure 11 do say the following:

Note(s):
1. Specified by design and characterization; not production tested.

[dhalbert]

There is a thorough implementation for the Synopsys DW_apb_i2c in the Linux kernel:
https://github.com/torvalds/linux/blob/master/drivers/i2c/busses/, files i2c-designware-*.c.

[kilograham]

cc @Wren6991

[dhalbert]

I have verified this fix works for the TC34725 and does not seem to interfere with the other I2C sensors I tried. I took Saleae traces and cannot see any timing differences, but without the fix the TCS34725 does not ACK several writes sent to it, and it works fine with the fix. We plan to incorporate this fix into CircuitPython's fork of pico-sdk for now.

[dhalbert]

We (CircuitPython) have also seen some flakiness with SH1107-based I2C OLED displays. This fix also seems to fix the SH1107 problems.

EDIT: A problem with SSD1306 OLED displays is also fixed by this.

[dhalbert]

Increasing IC_SDA_TX_HOLD to 5 instead of 2 will support the PA1010D GPS as well (#252 (comment)).

[fivdi]

On page 48 of the I2C-bus specification and user manual Table 10 lists the "Characteristics of the SDA and SCL bus lines for Standard, Fast, and Fast-mode Plus I2C-bus devices". Below this table note [3] mentions the following:

[3] A device must internally provide a hold time of at least 300 ns for the SDA signal (with respect to the V IH(min) of the SCL signal) to bridge the undefined region of the falling edge of SCL.

On page 511 of the RP2040 Datasheet the documentation for the IC_SDA_HOLD register mentions the following:

The bits [15:0] of this register are used to control the hold time of SDA during transmit in both slave and master mode
(after SCL goes from HIGH to LOW).

It would also be interesting to know what the DW_apb_i2c documentation has to say, but I don't have access to the documentation.

Should the value programmed in IC_SDA_TX_HOLD be what every is necessary to result in a 300 ns hold time?

[dhalbert]

It would also be interesting to know what the DW_apb_i2c documentation has to say, but I don't have access to the documentation.

I think what is in the RP2040 datasheet is just copied from the DW_apb_i2c documentation; it says so at the top.

Should the value programmed in IC_SDA_TX_HOLD be what every is necessary to result in a 300 ns hold time?

That makes perfect sense; the value can be calculated instead of this empirical testing. Would you like to revise the PR to do that?

The Linux driver mentions 300ns:
https://github.com/torvalds/linux/blob/a5e13c6df0e41702d2b2c77c8ad41677ebb065b3/drivers/i2c/busses/i2c-designware-master.c#L54-L56:

	/* Set standard and fast speed dividers for high/low periods */
	sda_falling_time = t->sda_fall_ns ?: 300; /* ns */
	scl_falling_time = t->scl_fall_ns ?: 300; /* ns */

[dhalbert]

Would you like to revise the PR to do that?

I made a PR for the Adafruit fork of pico-sdk: adafruit#3. It calculates the SDA hold time. There's a Saleae screenshot included showing the difference.

[fivdi]

Would you like to revise the PR to do that?

I'll revise it today.

[fivdi]

The PR has been revised.

[dhalbert]

@tannewt pointed out in our own PR that for "Fast Mode +", which is a 1MHz SCL clock rate, the specified fall time is only 120ns, so IC_SDA_TX_HOLD need only be 120ns or just over rather than 300ns. We did not hold up our own PR for this, but it could be added to this PR. The way the pico-sdk driver is written, it uses the "fast mode" setting of the hardware for all possible clock speeds. However, I don't know that the other calculations are applicable to 1MHz either.

I have never actually seen anyone use a 1MHz clock with CircuitPython, so if this is a limitation, we can live with it for now.

[fivdi]

@tannewt pointed out in our own PR that for "Fast Mode +", which is a 1MHz SCL clock rate, the specified fall time is only 120ns, so IC_SDA_TX_HOLD need only be 120ns or just over rather than 300ns. We did not hold up our own PR for this, but it could be added to this PR. The way the pico-sdk driver is written, it uses the "fast mode" setting of the hardware for all possible clock speeds. However, I don't know that the other calculations are applicable to 1MHz either.

I have never actually seen anyone use a 1MHz clock with CircuitPython, so if this is a limitation, we can live with it for now.

@dhalbert Thank you for the information. I'll look into it.

[fivdi]

I did some tests with a TCS34725 and a BME280 using the 300 ns SDA TX hold time in Fast-mode Plus and there were no issues. The wording in the specification doesn't explicitly mention what the SDA TX hold time should be in Fast-mode Plus. However, given that the fall time of both SDA and SCL signals in Fast-mode Plus is 120 ns a hold time of 120ns is more appropriate. This application note is also good. The PR has been updated to set the SDA TX hold time to 120 ns in Fast-mode Plus.

[lurch]

This looks reasonable to me, and it's encouraging to see that it fixes some of the reported problems with I2C, but I don't know enough about I2C to be able to give it a "formal approval" 🙂

Oh, but could you squash the commits please, as conceptually this is only a single change.

[Wren6991]

Thank you, this is a good change as per spec, and calculations look correct to me. I did to a double take on 25e6 vs 2.5e6, but seems correct.

[dhalbert]

@fivdi Thank you for your initial idea about this and your detailed spec reading to find the 300ns/120ns values.

[fivdi]

@FiViDi Thank you for your initial idea about this and your detailed spec reading to find the 300ns/120ns values.

@dhalbert You're welcome 😄