I2C Broadcom bug workaround.

[rewolff]

The broadcom BCM2835 has a (hardware) bug in the I2C module.

The BCM implements "clock stretching" in a bad way:

Instead of ensuring a minimum "hightime" for the clock, the BCM lets the I2C clock internally run on, and when it's time to flip the clock again, it checks wether or not the clock has become high, and if it is low, it does a "clock stretch". However if just in the few nanoseconds before this check the slave releases the clock signal indicating it is ready for a new clock, the clock pulse may become as short as 40ns. (that's the lowest I'm able to measure, and that of course is quite rare). With a 100kHz (10 microseconds) I2C clock, the "bad" moments to release the clock are 5 microseconds apart.

My slaves require a clock pulse of at least 500ns. (which should be a factor of ten shorter than what the master should generate when it is configured for 100kHz operation).

That is a hardware bug that we can't do much about at the moment. ( Tip for the hardware guys: instead of implementing clock stretching in this way: inhibit the counting of the CLOCKDIV counter while the clock remains low. This will probably clock-stretch a few fast-clocks on every cycle, but standards conformance will improve substantially).

When this occurs, the BCM will send a byte, with a very short first clock pulse. The slave then doesn't see the first clock pulse, so when at the end of that byte it is time to ACK that byte, the slave is still waiting for the 8th bit. The BCM interprets this as a "NAK" and aborts the I2C transfer. However at that point in time the slave sees its 8th clock pulse and issues an "ACK" (Pulls the SDA line low). This in turn means that the BCM cannot issue a proper "START" condition for the next transfer.

Suggested software workaround to reduce the impact of this hardware bug: When a transfer is aborted due to a "NAK" for a byte, issue a single extra clock cycle before the STOP to synchronize the slave. This probably has to be done in software.

This only happens in the "error path" when the slave doesn't ACK a byte. Maybe it's best to only do this when it is NOT the first byte in a transfer, as the bug cannot happen on the first byte of a transfer.

Does this software fix belong in the driver? Yes. It's a driver for a piece of hardware that has a bug. It is the duty of the driver to make this buggy hardware work the best it can.

[popcornmix]

If a workaround in the driver makes it work better for the majority of users, then sure it should be included.
Do you have a suitable patch?

[rewolff]

No. Not yet.

1. I have not delved into the I2C driver yet. Around a year ago, I had plans to write it myself but before I got around to it, someone else had written it. So someone who already knows this driver might be better equipped to do this as a five minute project. It would take me at least several hours to get to know the driver.
2. It pays to first discuss it if you (and maybe others) agree that a fix/workaround is warranted.
3. I just wanted to document this as a "todo" we shouldn't forget it....
4. Maybe someone would suggest a different workaround.

Speaking about workarounds: The workaround could also be triggered by the SDA line still being low when we are trying to initiate a new transaction. That is clearly a "problem" situation.

The "it goes wrong" case might be rare for "normal" cases. Many people will have a hardware-i2c module on the I2C-bus. Back when I2C was invented "10us" might have been a "very short" timeframe for some chips to handle the requests, so clock stretching was necessary. Nowadays any hardware I2C implementation should be able to handle "fast I2C" up to even faster.

This combines with the way the atmel guys implemented their async modules. Instead of having the module run on the externally provided clock (i2c-slave or SPI-slave), the module still runs on the processor clock. And it synchronizes all incoming signals by passing them through a bunch of filpflops. Good hardware design. The alternative is IMHO better: run the module off the external clock, and synchronize when the data passes to the other (cpu-) clock domain. This would for example allow the SPI module to function at 20MHz, even though the CPU runs at only 8.

Anyway. Enough hardware rambling.

Suggested workaround: Issue an extra clock cycle when at the beginning of a transaction the SDA line is still low. (it should start out high to be able to issue a "start" condition).

[popcornmix]

Firstly, I know little about I2C. I have spoken to Gert and others about workarounds.

There are two problems with I2C known about:

1. The clock stretching problem you have described
2. Problem with I2C state machine when doing restart

For 1, Gert's view is there is no foolproof workaround.
If I2C slave doesn't support clock stretching you are fine.
If I2C clock stretches by a bounded amount, then lowering the I2C clock frequency can avoid the problem
Switching to a bit banging driver for slaves that don't fall in the previous cases is a safe workaround.

I don't know if your proposal will work. I'd be interested to hear if it reduces the problem, or fixes it completely.

Gert says he has a desciption of the bug that he'll try and get released.

For 2, in the GPU I2C driver we have some code working around a problem with state machine:

/***********************************************************
 * Name: i2c_actual_read
 *
 * Arguments:
 *       const I2C_PERIPH_SETUP_T *periph_setup,
         const uint32_t sub_address,
         const uint32_t data_to_read_in_bytes,
         void *data
 *
 * Description: Routine to actually transfer data to the I2C peripheral
 *
 * Returns: int == 0 is success, all other values are failures
 *
 ***********************************************************/
static int32_t i2c_actual_read(  const I2C_PERIPH_SETUP_T *periph_setup,
                                 const I2C_USER_T *user,
                                 const uint32_t sub_address,
                                 const uint32_t read_nbytes,
                                 void *data,
                                 const int using_interrupt)
{
   int32_t rc = -1; /* Fail by default */
   int32_t status = 0;
   int32_t nbytes;

   uint32_t sub;
   uint8_t* read_data = (uint8_t*)data;
   uint32_t data_read = 0;

   if( NULL != periph_setup && NULL != data ) {

      // confirm that the latch is held for this transaction
      assert( i2c_state.periph_latch[ periph_setup->port ] != rtos_latch_unlocked() );

      // -> START
      // -> Slave device address | WRITE
      // <- Ack
      // -> Sub address
      // <- Ack
      // -> Sub address
      // <- Ack
      // -> ReSTART
      // -> Slave device address | READ
      // <- Ack
      // <- Data[0]
      // -> Ack
      // <- Data[1]
      // -> nAck
      // -> STOP

      I2CC_x( periph_setup->port )     = I2CC_EN | I2CC_CLEAR; /* Enable I2C and clear FIFO */
      I2CS_x( periph_setup->port )     = I2CS_ERR | I2CS_DONE | I2CS_CLKT; /* clear ERROR and DONE bits */
      I2CA_x( periph_setup->port )     = periph_setup->device_address;

      sub = sub_address;
      nbytes = periph_setup->sub_address_size_in_bytes;

      if( 0 == nbytes ) {
         /* No subaddress to send - just issue the read */
      } else {
         /*
           See i2c.v: The I2C peripheral samples the values for rw_bit and xfer_count in the IDLE state if start is set.

           We want to generate a ReSTART not a STOP at the end of the TX phase. In order to do that we must
           ensure the state machine goes RACK1 -> RACK2 -> SRSTRT1 (not RACK1 -> RACK2 -> SSTOP1). 

           So, in the RACK2 state when (TX) xfer_count==0 we must therefore have already set, ready to be sampled:
           READ ; rw_bit     <= I2CC bit 0   -- must be "read"
           ST;    start      <= I2CC bit 7   -- must be "Go" in order to not issue STOP
           DLEN;  xfer_count <= I2CDLEN      -- must be equal to our read amount

           The plan to do this is:
           1. Start the sub-add]ress write, but don't let it finish (keep xfer_count > 0)
           2. Populate READ, DLEN and ST in preparation for ReSTART read sequence
           3. Let TX finish (write the rest of the data)
           4. Read back data as it arrives

         */
         assert( nbytes <= 16 );
         I2CDLEN_x( periph_setup->port )  = nbytes;
         I2CC_x( periph_setup->port )    |= I2CC_EN | I2CC_START; /* Begin, latch the WRITE and TX xfer_count values */

         /* Wait for us to leave the idle state */
         while( 0 == ( I2CS_x(periph_setup->port) & (I2CS_TA|I2CS_ERR) ) ) {
            _nop();
         }
      }

      /* Now we can set up the parameters for the read - they don't get considered until the TX xfer_count==0 */
      I2CDLEN_x( periph_setup->port )  = read_nbytes;
      I2CC_x( periph_setup->port )     |= I2CC_EN | I2CC_START | I2CC_READ;

      /* Let the TX complete by providing the sub-address */
      while( nbytes-- ) {
         I2CFIFO_x( periph_setup->port ) = sub & 0xFF; /* No need to check FIFO fullness as sub-address <= 16 bytes long */
         sub >>= 8;
      }

      /* We now care that the transmit portion has completed; the FIFO is shared and we mustn't read out
         any of the data we were planning on writing to the slave! */

      /* Wait for peripheral to get to IDLE or one of the two RX states - this way we *know* TX has completed or hit an error */
      {
         uint32_t state;
         bool_t state_transition_complete;
         bool_t error_detected;
         do { 
            state = (I2CS_x( periph_setup->port ) & 0xf0000000) >> 28;
            state_transition_complete = ((state == 0) || (state == 4) || (state == 5));
            error_detected = (I2CS_x(periph_setup->port) & (I2CS_ERR | I2CS_CLKT)) != 0;
         } while(!state_transition_complete && !error_detected);

         if (error_detected) {
            /* Clean up, and disable I2C */
            I2CC_x( periph_setup->port ) &= ~(I2CC_INTD | I2CC_INTR);
            I2CC_x( periph_setup->port ) &= ~(I2CC_START | I2CC_READ);
            I2CS_x( periph_setup->port ) = I2CS_CLKT | I2CS_ERR | I2CS_DONE;
            I2CC_x( periph_setup->port ) |= I2CC_CLEAR;
            I2CC_x( periph_setup->port ) = 0;
            return -1;
         }
      }

      if (using_interrupt)
      {
         /* Wait for interrupt to complete. */
         i2c_state.active_buffer[periph_setup->port] = data;
         i2c_state.active_buffer_length[periph_setup->port] = read_nbytes;
         i2c_state.active_buffer_offset[periph_setup->port] = 0;
         i2c_state.pending_transfer[periph_setup->port] = I2C_PENDING_TRANSFER_READ;
         RTOS_LATCH_T latch = rtos_latch_locked();
         i2c_state.pending_latch[periph_setup->port] = &latch;

         /* Enable interrupt. */
         I2CC_x( periph_setup->port ) |= I2CC_INTD | I2CC_INTR;

         rtos_latch_get (&latch);

         i2c_state.pending_latch[periph_setup->port] = NULL;
         data_read = i2c_state.active_buffer_offset[periph_setup->port];

         rc = (data_read == read_nbytes) ? 0 : -1;
      }
      else
      {
         uint32_t time_now = 0;

         /* Loop until we've read all our data or failed. */
         while( 0 == ( I2CS_x(periph_setup->port) & (I2CS_TA|I2CS_ERR|I2CS_DONE) ) ) {
            _nop();
         }

         /* Wait for some data to arrive - we should wait, at most, I2C_TIMEOUT_IN_USECS for data to arrive every time we start waiting */
         time_now = i2c_state.systimer_driver->get_time_in_usecs( i2c_state.systimer_handle );
         while( ((i2c_state.systimer_driver->get_time_in_usecs( i2c_state.systimer_handle ) - time_now) < I2C_TIMEOUT_IN_USECS)
                && ( data_read < read_nbytes )
                && !(I2CS_x( periph_setup->port ) & I2CS_ERR) ) 
         {
            if (I2CS_x( periph_setup->port ) & I2CS_RXD) 
            {
               read_data[ data_read ] = I2CFIFO_x( periph_setup->port );
               data_read++;
               time_now = i2c_state.systimer_driver->get_time_in_usecs( i2c_state.systimer_handle  );
            }
         }

         if( (data_read != read_nbytes) /* Did we read all the data we asked for? */
             || ( (read_nbytes - data_read) != I2CDLEN_x( periph_setup->port ) ) /* Has DLEN decremented? */
             || ( 0 != (I2CS_x( periph_setup->port ) & I2CS_ERR) ) ) { /* Are there any errors? */
            rc = -1;
         } else {
            while( I2CS_DONE != (I2CS_x(periph_setup->port) & I2CS_DONE) ); /* Wait for the peripheral */
            rc = 0;
         }
      }

      /* Clean up, and disable I2C */
      I2CC_x( periph_setup->port ) &= ~(I2CC_INTD | I2CC_INTR);
      if(I2CS_x( periph_setup->port ) & I2CS_ERR) {
         //Wait for it to be idle
         while(I2CS_x( periph_setup->port ) & I2CS_TA)
            _nop();
      }
      I2CS_x( periph_setup->port ) = I2CS_ERR | I2CS_DONE;
      //Finally disable the I2C
      I2CC_x( periph_setup->port ) = 0x0;
   }

   if( !user->skip_asserts ) {
      assert( rc >= 0 );
      _nop();
   }

   return rc;
}      

I'm not sure if people are hitting this issue, but the pasted could may be helpful if you are.

[rewolff]

It is not possible to completely eradicate this hardware bug.

When it happens, the master and slave disagree on the number of clock cycles. Therefore, the data, as interpreted by the slave and the data as the master (= raspberry pi = driver/application) intended differ. You might be writing a completely bogus value in a register somewhere. Highly annoying.

My suggested workaround would at least cause the next transaction to work. The impact would be halved: only one transaction botched instead of two. It would not eliminate the problem.

If I2C slave doesn't support clock stretching you are fine.

If the I2C slave doesn't require clock stretching....

If I2C clock stretches by a bounded amount, then lowering the I2C clock frequency can avoid the problem

Yes. I have 10 devices "in the field" at one location. Apparently the clock frequency (RC clock) of three of the modules is different in such a way that they end up "done" with the clock stretching in exactly the wrong moment. Reducing the clock frequency fixed the problems for those modules.

If the clock stretching delay is NOT fixed, but, say, varies by more than 5 microseconds, there is a more or less statistical chance of hitting the problem. In my case the slave requires 0.25 microseconds of clock width. So clock stretching ends in the 5% of the time leading up to the clock changing moment for the BCM, then things go wrong. So, in this case, about 1 in 20 transfers would go wrong. (specs say min 250ns for a clock pulse. That in fact doesn't mean that it won't be seen if it is shorter. I think the chances of it being seen goes linearly from 100% at > 250ns to 0% at < 125ns. )

I've spent yesterday stretching the clock stretching period in such a way that it would fall around the 2.5 microsecond mark in the 5 microsecond window-of-opportunity. If I'd aim for the start of the window and my RC clock runs bit fast, I'd hit the "0" which triggers the bug. If I aim for 5, same thing. So now I'm aiming in the middle, away from the bad places. But change the I2C clock to 80kHz, and BAM, I'm aiming right for the sensitive spot... (and if it doesn't happen with 80kHz, it'll happen with some value between 80 and 100).

[popcornmix]

Here is Gert's description of the I2C bug:
https://dl.dropbox.com/u/3669512/2835_I2C%20interface.pdf

[rewolff]

Haha! I have instrumented my test-board in exactly the same way. I've cut the SCL trace and mounted a 0.1" jumper connector on there. While not measuring there is a normal jumper there, or a female connector with a 100 ohm resistor when I am.

I don't think it only happens on the first clock. That would mean I could work around the problem by either NOT clock stretching at all (as most hardware I2C chips probably do), or by causing the clock stretch to last at least a full cycle.

I have instrumented my i2c slave code in such a way that I can cause the clock stretch to change by 0.5 microsecond increments. I can command it to change the clock stretch to XXX over I2C and then it works like that for 50 miliseconds before changing back to the default values. (in case it doesn't work we need it working again. This way I can quickly scan a whole range of values for these settings, while monitoring the SDA and SCL lines with a logic analyzer. Sending a "use delay XXX" and then sending a sting takes 25 miliseconds. THen incrementing a counter in the script and sending the "next delay" takes the shell script about 100ms. So I can scan 100 possible values in about 10 seconds.

I HAVE seen the bug happening on longer delays. So the theory: "it only happens on the first" is not correct.

As to the "super efficient" interrupt handler: Yes, I can do that too. The thing is: The clock stretching feature allows me to put the "handle this byte" code in the interrupt routine and not worry about it taking a few microseconds or a few miliseconds. In theory the clock stretching will make sure that the master waits for me to finish handling this byte before continuing with the next one. That plan is totally busted with the bug in the 2835. I now just cause clock stretching to stop, and then hope to finish handling the byte in the 70 microseconds that follow.

I have to "cycle count" to aim for the middle of the 5 microsecond half-clock for me to release the clock stretching. If I'd aim for the start (to cause a 4.9 microsecond clock-high-period) the clock on my slave might be running a few percent faster and cause the bug to trigger a half-cycle earlier. So both ends of the half-clock cycle are dangerous, I have to aim for a short, but valid enough clock cycle.

The change in the module is IMHO simple: When SCL is "driven high" (i.e. not driven at all), stop the I2C module master clock. This has the side effect of allowing much longer I2C buses because automatic clock stretching occurs when the bus capacitance is above spec.

[iz8mbw]

This: http://comments.gmane.org/gmane.linux.kernel.rpi/268 and this: http://news.gmane.org/gmane.linux.kernel.rpi can help?

[rewolff]

Thanks for trying to help, iz8mbw, but those have nothing to do with this (hardware) bug.

I've measured my I2C clock and it comes out as 100kHz which the driver thinks it should be. The driver has a "clock source" object which it queries for the frequency and it uses that. I have not yet instrumented my driver to print out the frequency.

[rewolff]

Just for information for people who want to SEE the bug in action I have some "proof" in the form of a logic analsyer dump.
Hmm I can attach images, but not the binary LA dump. That one is here: http://prive.bitwizard.nl/LA_with_bug_scan_50_to_100_microseconds_delay.bin.gz

In the image LA_with_bug_shot1.png you'll see the short burst of activity: "set delay to XXX" then a small burst that tries to write some 10 bytes, but it gets aborted due to a NACK. On the right you see a proper 10-byte-packet. The SDA line remains low until the next burst that is misaligned (all bits are shifted, and in my slave the "xth-byte-in-packet" counter has not been reset by the non-existant start condition at the start of that packet.

In the image LA_with_bug_shot2.png I've zoomed in to the section where the short pulse happens. This the longest pulse that I've seen go wrong: 290ns. (above datasheet-spec for the Atmel!)

[rfmerrill]

Oh so THAT's where that came from!

I've got a project I'm trying to control with I2C from a raspberry pi and I'm honestly close to just giving up and either switching to a beagleboard or having the Pi talk to an FPGA via SPI which then does the I2C stuff.

[rewolff]

I have a board in the mail that should be able to do that (expected delivery: tomorrow). SPI connector, I2C connector. Would be a nice project to try with that board. Send me an email if you want to be kept informed.

Current situation: I'm seeing behaviour compatible with Gert's: "only the first cycle can go wrong" when the slave is an ATMEGA, and I'm seeing the bug in full force when the slave is one of my ATTINYs.

Here are the delays I tested:

and the shortest SCL pulse observed during this test-session was 4.5 microseconds.

Now testing with the attiny slave, the tested delays for the ACK are:

and the minimum pulse width is 41ns (my measurement resolution):

Ah. Found it! It seems that clock stretching before the ACK of a byte is "dangerous" but clock stretching AFTER the ACK is ok.....

[markdootson]

I and others are certainly hitting the repeated start issue. I have a userspace hack at http://cpansearch.perl.org/src/MDOOTSON/HiPi-0.26/BCM2835.xs in proc hipi_i2c_read_register_rs. This seems to work for the users who have reported trying it or incorporating the C code ( though that's only 5 people ), even though I had not figured out correctly how to determine that the TX stage had completed. I suppose I need to learn to compile kernel modules and run existing tests against code above.

[Ferroin]

Couldn't we just force the kernel to ignore the I2C hardware and use bitbanged GPIO-based I2C on the same pins? I would think the overhead for that would be more acceptable then the bugs in the hardware itself.

[rewolff]

What overhead do you expect? The current driver defaults to 100kHz clock. So reading 10 bytes from an i2c intertial sensor takes about 1ms. If you bitbang, you're likely to be churning CPU cycles all that time....

[popcornmix]

My preference would be for the driver to optionally support bitbanging i2c through a module parameter (or maybe a separate module if that is more convenient).

Many devices don't use clock stretching (or clock stretch by a bounded amount so can work correctly at reduced speed), so the hardware I2C driver is still preferable in those cases.