i2c_sam0 driver does not execute a STOP condition

[sslupsky]

Describe the bug
The i2c_sam0 driver does not execute a STOP condition. As a result, at the end of each transaction, the SCL line is forced low.

To Reproduce
Steps to reproduce the behavior:

1. use blinky example
2. add an i2c peripheral
3. make
4. observe the SCL line
5. See error

Expected behavior
SCL should be released (go high) when a bus transaction is complete.

Impact
When SCL is held low, the pull up resistors on the bus consume power on the order of 500uA or more (depending on the value of the pull up resistor) indefinitely. This bug rules out any low power applications that require the i2c driver.

Additional context
The i2c CMD bits can only be set when either the Slave on Bus interrupt flag (INTFLAG.SB) or Master on Bus interrupt flag (INTFLAG.MB) is '1'. See 28.10.2. So, the stop command must be issued before the MB and SB interrupt flag bits are cleared. See 28.10.6. Otherwise, the clock (SCL) will be held low.

I created a POC to verify the expected behaviour. Writing the data register or reading the data register when SBEN is enabled clears SB and MB. Thus, we do not need to clear MB or SB in the ISR. Thus, the following line was removed from the i2c_sam0_isr() in my POC:

	i2c->INTFLAG.reg = status;

The end of RX is not handled properly so the following code segment was reorganized in the i2c_sam0_isr():

	if (status & SERCOM_I2CM_INTFLAG_SB) {
		if (!data->msg.size) {
			i2c->INTENCLR.reg = SERCOM_I2CM_INTENCLR_MASK;
			k_sem_give(&data->sem);
			return;
		}

		if (data->msg.size == 1) {
			/*
			 * If this is the last byte, then prepare for an auto
			 * NACK before doing the actual read.  This does not
			 * require write synchronization.
			 */
			i2c->CTRLB.bit.ACKACT = 1;
		}

		*data->msg.buffer = i2c->DATA.reg;
		data->msg.buffer++;
		data->msg.size--;

		return;
	}

The error flag is cleared in i2c_sam0_terinate_on_error():

	i2c->INTFLAG.reg = SERCOM_I2CM_INTFLAG_ERROR;

This POC shows that the STOP condition is properly executed at the end of a bus transaction.

[jhedberg]

@mnkp is there anything you could do to help with this one?

[sslupsky]

@jhedberg I have a PR pending to address this and several other issues with the sam0 i2c driver. The reason this has taken quite some time to resolve is the inter relationship with the low power modes of the sam0 and the rtc_timer driver. I've addressed both and will submit PR's soon.

BTW, this is a pretty serious issue. I2C is pretty much unusable on sam0.

[sslupsky]

@MaureenHelm PR 24050 has been rebased.