[PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes

[PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes

[Noralf Trønnes]

Writing messages larger than the FIFO size results in a hang, rendering
the machine unusable. This is because the RXD status flag is set on the
first interrupt which results in bcm2835_drain_rxfifo() stealing bytes
from the buffer. The controller continues to trigger interrupts waiting
for the missing bytes, but bcm2835_fill_txfifo() has none to give.
In this situation wait_for_completion_timeout() apparently is unable to
stop the madness.

The BCM2835 ARM Peripherals datasheet has this to say about the flags:
  TXD: is set when the FIFO has space for at least one byte of data.
  RXD: is set when the FIFO contains at least one byte of data.
  TXW: is set during a write transfer and the FIFO is less than full.
  RXR: is set during a read transfer and the FIFO is or more full.

Implementing the logic from the downstream i2c-bcm2708 driver solved
the hang problem.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/i2c/busses/i2c-bcm2835.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index d4f3239..f283b71 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -64,6 +64,7 @@ struct bcm2835_i2c_dev {
 	int irq;
 	struct i2c_adapter adapter;
 	struct completion completion;
+	struct i2c_msg *curr_msg;
 	u32 msg_err;
 	u8 *msg_buf;
 	size_t msg_buf_remaining;
@@ -126,14 +127,13 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 		return IRQ_HANDLED;
 	}
 
-	if (val & BCM2835_I2C_S_RXD) {
-		bcm2835_drain_rxfifo(i2c_dev);
-		if (!(val & BCM2835_I2C_S_DONE))
-			return IRQ_HANDLED;
-	}
-
 	if (val & BCM2835_I2C_S_DONE) {
-		if (i2c_dev->msg_buf_remaining)
+		if (i2c_dev->curr_msg->flags & I2C_M_RD) {
+			bcm2835_drain_rxfifo(i2c_dev);
+			val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
+		}
+
+		if ((val & BCM2835_I2C_S_RXD) || i2c_dev->msg_buf_remaining)
 			i2c_dev->msg_err = BCM2835_I2C_S_LEN;
 		else
 			i2c_dev->msg_err = 0;
@@ -141,11 +141,16 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 		return IRQ_HANDLED;
 	}
 
-	if (val & BCM2835_I2C_S_TXD) {
+	if (val & BCM2835_I2C_S_TXW) {
 		bcm2835_fill_txfifo(i2c_dev);
 		return IRQ_HANDLED;
 	}
 
+	if (val & BCM2835_I2C_S_RXR) {
+		bcm2835_drain_rxfifo(i2c_dev);
+		return IRQ_HANDLED;
+	}
+
 	return IRQ_NONE;
 }
 
@@ -155,6 +160,7 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 	u32 c;
 	unsigned long time_left;
 
+	i2c_dev->curr_msg = msg;
 	i2c_dev->msg_buf = msg->buf;
 	i2c_dev->msg_buf_remaining = msg->len;
 	reinit_completion(&i2c_dev->completion);
-- 
2.8.2

[PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Noralf Trønnes]

Some SMBus protocols use Repeated Start Condition to switch from write
mode to read mode. Devices like MMA8451 won't work without it.

When downstream implemented support for this in i2c-bcm2708, it broke
support for some devices, so a module parameter was added and combined
transfer was disabled by default.
See https://github.com/raspberrypi/linux/issues/599
It doesn't seem to have been any investigation into what the problem
really was. Later there was added a timeout on the polling loop.

One of the devices mentioned to partially stop working was DS1307.

I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
and AT24C32 (eeprom) in parallel without problems.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/i2c/busses/i2c-bcm2835.c | 107 +++++++++++++++++++++++++++++++++++----
 1 file changed, 98 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index f283b71..b3ce565 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -154,8 +154,39 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 	return IRQ_NONE;
 }
 
+/*
+ * Repeated Start Condition (Sr)
+ * The BCM2835 ARM Peripherals datasheet mentions a way to trigger a Sr when it
+ * talks about reading from a slave with 10 bit address. This is achieved by
+ * issuing a write (without enabling interrupts), poll the I2CS.TA flag and
+ * wait for it to be set, and then issue a read.
+ * https://github.com/raspberrypi/linux/issues/254 shows how the firmware does
+ * it and states that it's a workaround for a problem in the state machine.
+ * This is the comment in the firmware code:
+ *
+ *     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-address 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
+ */
+
 static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
-				struct i2c_msg *msg)
+				struct i2c_msg *msg, struct i2c_msg *msg2)
 {
 	u32 c;
 	unsigned long time_left;
@@ -167,21 +198,70 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 
 	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
 
-	if (msg->flags & I2C_M_RD) {
-		c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
-	} else {
-		c = BCM2835_I2C_C_INTT;
+	if (!(msg->flags & I2C_M_RD))
 		bcm2835_fill_txfifo(i2c_dev);
-	}
-	c |= BCM2835_I2C_C_ST | BCM2835_I2C_C_INTD | BCM2835_I2C_C_I2CEN;
 
 	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_A, msg->addr);
 	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg->len);
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+
+	if (!msg2) {
+		if (msg->flags & I2C_M_RD)
+			c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
+		else
+			c = BCM2835_I2C_C_INTT;
+
+		c |= BCM2835_I2C_C_ST | BCM2835_I2C_C_INTD |
+		     BCM2835_I2C_C_I2CEN;
+		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+	} else {
+		unsigned long flags;
+		u32 stat, err = 0;
+
+		local_irq_save(flags);
+
+		/* Start write message */
+		c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN;
+		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+
+		/* Wait for the transfer to become active */
+		for (time_left = 100; time_left > 0; time_left--) {
+			stat = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
+
+			err = stat & (BCM2835_I2C_S_CLKT | BCM2835_I2C_S_ERR);
+			if (err)
+				break;
+
+			if (stat & BCM2835_I2C_S_TA)
+				break;
+		}
+
+		if (err || !time_left) {
+			i2c_dev->msg_err = err;
+			local_irq_restore(flags);
+			goto error;
+		}
+
+		/* Start read message */
+		i2c_dev->curr_msg = msg2;
+		i2c_dev->msg_buf = msg2->buf;
+		i2c_dev->msg_buf_remaining = msg2->len;
+		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DLEN, msg2->len);
+
+		c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR |
+		    BCM2835_I2C_C_INTD | BCM2835_I2C_C_ST |
+		    BCM2835_I2C_C_I2CEN;
+
+		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, c);
+
+		local_irq_restore(flags);
+	}
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						BCM2835_I2C_TIMEOUT);
+error:
 	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
+	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, BCM2835_I2C_S_CLKT |
+			   BCM2835_I2C_S_ERR | BCM2835_I2C_S_DONE);
 	if (!time_left) {
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
 		return -ETIMEDOUT;
@@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 	int i;
 	int ret = 0;
 
+	/* Combined write-read to the same address (smbus) */
+	if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+	    !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+	    (msgs[0].len <= 16)) {
+		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
+
+		return ret ? ret : 2;
+	}
+
 	for (i = 0; i < num; i++) {
-		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
+		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
 		if (ret)
 			break;
 	}
-- 
2.8.2

[PATCH 3/3] i2c: bcm2835: Use ratelimited logging on transfer errors

[Noralf Trønnes]

Writing to an AT24C32 generates on average 2x i2c transfer errors per
32-byte page write. Which amounts to a lot for a 4k write. This is due
to the fact that the chip doesn't respond during it's internal write
cycle when the at24 driver tries and retries the next write.
Reduce this flooding of the log by using dev_err_ratelimited().

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---
 drivers/i2c/busses/i2c-bcm2835.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index b3ce565..dc0a5d5 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -274,7 +274,8 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 	    (msg->flags & I2C_M_IGNORE_NAK))
 		return 0;
 
-	dev_err(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
+	dev_err_ratelimited(i2c_dev->dev, "i2c transfer failed: %x\n",
+			    i2c_dev->msg_err);
 
 	if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
 		return -EREMOTEIO;
-- 
2.8.2

Re: [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

Noralf Trønnes <noralf@tronnes.org> writes:

> Writing messages larger than the FIFO size results in a hang, rendering
> the machine unusable. This is because the RXD status flag is set on the
> first interrupt which results in bcm2835_drain_rxfifo() stealing bytes
> from the buffer. The controller continues to trigger interrupts waiting
> for the missing bytes, but bcm2835_fill_txfifo() has none to give.
> In this situation wait_for_completion_timeout() apparently is unable to
> stop the madness.
>
> The BCM2835 ARM Peripherals datasheet has this to say about the flags:
>   TXD: is set when the FIFO has space for at least one byte of data.
>   RXD: is set when the FIFO contains at least one byte of data.
>   TXW: is set during a write transfer and the FIFO is less than full.
>   RXR: is set during a read transfer and the FIFO is or more full.
>
> Implementing the logic from the downstream i2c-bcm2708 driver solved
> the hang problem.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>

Patches 1, 3 are:

Reviewed-by: Eric Anholt <eric@anholt.net>

For patch 2 I followed some of it, but couldn't quite say it's a review.
I trust your testing, and that the path has had a lot more testing in
the downstream tree, so it's:

Acked-by: Eric Anholt <eric@anholt.net>

Thanks!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes

[Noralf Trønnes]

Den 19.09.2016 18:51, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> Writing messages larger than the FIFO size results in a hang, rendering
>> the machine unusable. This is because the RXD status flag is set on the
>> first interrupt which results in bcm2835_drain_rxfifo() stealing bytes
>> from the buffer. The controller continues to trigger interrupts waiting
>> for the missing bytes, but bcm2835_fill_txfifo() has none to give.
>> In this situation wait_for_completion_timeout() apparently is unable to
>> stop the madness.
>>
>> The BCM2835 ARM Peripherals datasheet has this to say about the flags:
>>    TXD: is set when the FIFO has space for at least one byte of data.
>>    RXD: is set when the FIFO contains at least one byte of data.
>>    TXW: is set during a write transfer and the FIFO is less than full.
>>    RXR: is set during a read transfer and the FIFO is or more full.
>>
>> Implementing the logic from the downstream i2c-bcm2708 driver solved
>> the hang problem.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> Patches 1, 3 are:
>
> Reviewed-by: Eric Anholt <eric@anholt.net>
>
> For patch 2 I followed some of it, but couldn't quite say it's a review.
> I trust your testing, and that the path has had a lot more testing in
> the downstream tree, so it's:

I don't know how much testing downstream really has had since this feature
is disabled by default. So there is a possibility that this can break some
devices (while at the same time enable others).
I wondered about adding a module parameter so it could be disabled at
runtime, but decided to see what the review would be first.
It is of course possible to add a this module parameter later should it
turn out to break some device.

Noralf.

> Acked-by: Eric Anholt <eric@anholt.net>
>
> Thanks!

Re: [PATCH 1/3] i2c: bcm2835: Fix hang for writing messages larger than 16 bytes

[Martin Sperl]

On 19.09.2016 17:26, Noralf Trønnes wrote:
> Writing messages larger than the FIFO size results in a hang, rendering
> the machine unusable. This is because the RXD status flag is set on the
> first interrupt which results in bcm2835_drain_rxfifo() stealing bytes
> from the buffer. The controller continues to trigger interrupts waiting
> for the missing bytes, but bcm2835_fill_txfifo() has none to give.
I remember having seen similar interrupt issues with the SPI HW-block.
> In this situation wait_for_completion_timeout() apparently is unable to
> stop the madness.
That is because it is a level irq that immediately triggers another 
interrupt giving
no CPU no time to do other (threaded) OS activity - this might be slightly
different for multi-core machines...
> The BCM2835 ARM Peripherals datasheet has this to say about the flags:
>    TXD: is set when the FIFO has space for at least one byte of data.
>    RXD: is set when the FIFO contains at least one byte of data.
>    TXW: is set during a write transfer and the FIFO is less than full.
>    RXR: is set during a read transfer and the FIFO is or more full.
>
> Implementing the logic from the downstream i2c-bcm2708 driver solved
> the hang problem.
>
> Signed-off-by: Noralf Trønnes<noralf@tronnes.org>
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Martin Sperl]

Hi Noralf!

On 19.09.2016 17:26, Noralf Trønnes wrote:
> Some SMBus protocols use Repeated Start Condition to switch from write
> mode to read mode. Devices like MMA8451 won't work without it.
>
> When downstream implemented support for this in i2c-bcm2708, it broke
> support for some devices, so a module parameter was added and combined
> transfer was disabled by default.
> See https://github.com/raspberrypi/linux/issues/599
> It doesn't seem to have been any investigation into what the problem
> really was. Later there was added a timeout on the polling loop.
>
> One of the devices mentioned to partially stop working was DS1307.
>
> I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
> and AT24C32 (eeprom) in parallel without problems.
>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>   drivers/i2c/busses/i2c-bcm2835.c | 107 +++++++++++++++++++++++++++++++++++----
>   1 file changed, 98 insertions(+), 9 deletions(-)
...
> @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
>   	int i;
>   	int ret = 0;
>   
> +	/* Combined write-read to the same address (smbus) */
> +	if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
> +	    !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
> +	    (msgs[0].len <= 16)) {
> +		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
> +
> +	return ret ? ret : 2;
> +	}
> +
>   	for (i = 0; i < num; i++) {
> -		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
> +		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
>   		if (ret)
>   			break;
>   	}
This does not seem to implement the i2c_msg api correctly.

As per comments in include/uapi/linux/i2c.h on line 58 only the last message
in a group should - by default - send a STOP.

As far as I understand  you would need to implement the I2C_M_STOP flag
(by exposing  I2C_FUNC_PROTOCOL_MANGLING in bcm2835_i2c_func)
to make this work correctly:

  	for (i = 0; i < num; i++) {
+		bool send_stop = (i == num - 1) ||msgs[i] <http://lxr.free-electrons.com/ident?i=msg>->flags <http://lxr.free-electrons.com/ident?i=flags>  &I2C_M_STOP <http://lxr.free-electrons.com/ident?i=I2C_M_STOP>;
-		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
+		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], send_stop);
  		if (ret)
  			break;
  	}

The corresponding device driver (or userspace) will need to set the flag 
correctly.

Martin

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Noralf Trønnes]

Den 20.09.2016 09:19, skrev Martin Sperl:
> Hi Noralf!
>
> On 19.09.2016 17:26, Noralf Trønnes wrote:
>> Some SMBus protocols use Repeated Start Condition to switch from write
>> mode to read mode. Devices like MMA8451 won't work without it.
>>
>> When downstream implemented support for this in i2c-bcm2708, it broke
>> support for some devices, so a module parameter was added and combined
>> transfer was disabled by default.
>> See https://github.com/raspberrypi/linux/issues/599
>> It doesn't seem to have been any investigation into what the problem
>> really was. Later there was added a timeout on the polling loop.
>>
>> One of the devices mentioned to partially stop working was DS1307.
>>
>> I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
>> and AT24C32 (eeprom) in parallel without problems.
>>
>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>> ---
>>   drivers/i2c/busses/i2c-bcm2835.c | 107 
>> +++++++++++++++++++++++++++++++++++----
>>   1 file changed, 98 insertions(+), 9 deletions(-)
> ...
>> @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter 
>> *adap, struct i2c_msg msgs[],
>>       int i;
>>       int ret = 0;
>>   +    /* Combined write-read to the same address (smbus) */
>> +    if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>> +        !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>> +        (msgs[0].len <= 16)) {
>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>> +
>> +    return ret ? ret : 2;
>> +    }
>> +
>>       for (i = 0; i < num; i++) {
>> -        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
>>           if (ret)
>>               break;
>>       }
> This does not seem to implement the i2c_msg api correctly.
>
> As per comments in include/uapi/linux/i2c.h on line 58 only the last 
> message
> in a group should - by default - send a STOP.
>

Apparently it's a known problem that the i2c controller doesn't support
Repeated Start. It will always issue a Stop when it has transferred DLEN
bytes.
Refs:
http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they

UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
and before DONE is set (or the last byte is shifted, I don't know excatly).
Refs:
https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834

I found this answer/report by joan that the downstream combined support
isn't reliable:
http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they

My implementation differs from downstream in that I use local_irq_save()
to protect the polling loop. But that only protects from missing the TA
(downstream can miss the TA and issue a Stop).

So currently in mainline we have a driver that says it support the standard
(I2C_FUNC_I2C), but it really only supports one message transfers since it
can't do ReStart.

What I have done in this patch is to support ReStart for transfers with
2 messages: first write, then read. But maybe a better solution is to just
leave this alone if it is flaky and use bitbanging instead. I don't know.


Noralf.


> As far as I understand  you would need to implement the I2C_M_STOP flag
> (by exposing  I2C_FUNC_PROTOCOL_MANGLING in bcm2835_i2c_func)
> to make this work correctly:
>
>      for (i = 0; i < num; i++) {
> +        bool send_stop = (i == num - 1) ||msgs[i] 
> <http://lxr.free-electrons.com/ident?i=msg>->flags 
> <http://lxr.free-electrons.com/ident?i=flags> &I2C_M_STOP 
> <http://lxr.free-electrons.com/ident?i=I2C_M_STOP>;
> -        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], send_stop);
>          if (ret)
>              break;
>      }
>
> The corresponding device driver (or userspace) will need to set the 
> flag correctly.
>
> Martin

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Martin Sperl]

On 20.09.2016 10:41, Noralf Trønnes wrote:
>
> Den 20.09.2016 09:19, skrev Martin Sperl:
>> Hi Noralf!
>>
>> On 19.09.2016 17:26, Noralf Trønnes wrote:
>>> Some SMBus protocols use Repeated Start Condition to switch from write
>>> mode to read mode. Devices like MMA8451 won't work without it.
>>>
>>> When downstream implemented support for this in i2c-bcm2708, it broke
>>> support for some devices, so a module parameter was added and combined
>>> transfer was disabled by default.
>>> See https://github.com/raspberrypi/linux/issues/599
>>> It doesn't seem to have been any investigation into what the problem
>>> really was. Later there was added a timeout on the polling loop.
>>>
>>> One of the devices mentioned to partially stop working was DS1307.
>>>
>>> I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
>>> and AT24C32 (eeprom) in parallel without problems.
>>>
>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>> ---
>>>   drivers/i2c/busses/i2c-bcm2835.c | 107
>>> +++++++++++++++++++++++++++++++++++----
>>>   1 file changed, 98 insertions(+), 9 deletions(-)
>> ...
>>> @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter
>>> *adap, struct i2c_msg msgs[],
>>>       int i;
>>>       int ret = 0;
>>>   +    /* Combined write-read to the same address (smbus) */
>>> +    if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>>> +        !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>>> +        (msgs[0].len <= 16)) {
>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>>> +
>>> +    return ret ? ret : 2;
>>> +    }
>>> +
>>>       for (i = 0; i < num; i++) {
>>> -        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
>>>           if (ret)
>>>               break;
>>>       }
>> This does not seem to implement the i2c_msg api correctly.
>>
>> As per comments in include/uapi/linux/i2c.h on line 58 only the last
>> message
>> in a group should - by default - send a STOP.
>>
>
> Apparently it's a known problem that the i2c controller doesn't support
> Repeated Start. It will always issue a Stop when it has transferred DLEN
> bytes.
> Refs:
> http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>
>
> UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
> and before DONE is set (or the last byte is shifted, I don't know excatly).
> Refs:
> https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
> https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834
>
>
> I found this answer/report by joan that the downstream combined support
> isn't reliable:
> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>
>
> My implementation differs from downstream in that I use local_irq_save()
> to protect the polling loop. But that only protects from missing the TA
> (downstream can miss the TA and issue a Stop).
>
> So currently in mainline we have a driver that says it support the standard
> (I2C_FUNC_I2C), but it really only supports one message transfers since it
> can't do ReStart.
>
> What I have done in this patch is to support ReStart for transfers with
> 2 messages: first write, then read. But maybe a better solution is to just
> leave this alone if it is flaky and use bitbanging instead. I don't know.
I have not said that the approach you have taken is wrong or bad.

I was only telling you that the portion inside the bcm2835_i2c_xfer:
+	/* Combined write-read to the same address (smbus) */
+	if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
+	    !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
+	    (msgs[0].len <= 16)) {
+		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
+
+		return ret ? ret : 2;
+	}
is very specific and maybe could be done in a "generic" manner
supporting more cases.

At least add a dev_warn_once for all num > 1 cases not handled by the
code above.

This gives people an opportunity to detect such a situation if they
find something is not working as expected.

Martin

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Noralf Trønnes]

Den 20.09.2016 12:15, skrev Martin Sperl:
>
>
> On 20.09.2016 10:41, Noralf Trønnes wrote:
>>
>> Den 20.09.2016 09:19, skrev Martin Sperl:
>>> Hi Noralf!
>>>
>>> On 19.09.2016 17:26, Noralf Trønnes wrote:
>>>> Some SMBus protocols use Repeated Start Condition to switch from write
>>>> mode to read mode. Devices like MMA8451 won't work without it.
>>>>
>>>> When downstream implemented support for this in i2c-bcm2708, it broke
>>>> support for some devices, so a module parameter was added and combined
>>>> transfer was disabled by default.
>>>> See https://github.com/raspberrypi/linux/issues/599
>>>> It doesn't seem to have been any investigation into what the problem
>>>> really was. Later there was added a timeout on the polling loop.
>>>>
>>>> One of the devices mentioned to partially stop working was DS1307.
>>>>
>>>> I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
>>>> and AT24C32 (eeprom) in parallel without problems.
>>>>
>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>> ---
>>>>   drivers/i2c/busses/i2c-bcm2835.c | 107
>>>> +++++++++++++++++++++++++++++++++++----
>>>>   1 file changed, 98 insertions(+), 9 deletions(-)
>>> ...
>>>> @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter
>>>> *adap, struct i2c_msg msgs[],
>>>>       int i;
>>>>       int ret = 0;
>>>>   +    /* Combined write-read to the same address (smbus) */
>>>> +    if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>>>> +        !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>>>> +        (msgs[0].len <= 16)) {
>>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>>>> +
>>>> +    return ret ? ret : 2;
>>>> +    }
>>>> +
>>>>       for (i = 0; i < num; i++) {
>>>> -        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
>>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
>>>>           if (ret)
>>>>               break;
>>>>       }
>>> This does not seem to implement the i2c_msg api correctly.
>>>
>>> As per comments in include/uapi/linux/i2c.h on line 58 only the last
>>> message
>>> in a group should - by default - send a STOP.
>>>
>>
>> Apparently it's a known problem that the i2c controller doesn't support
>> Repeated Start. It will always issue a Stop when it has transferred DLEN
>> bytes.
>> Refs:
>> http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they 
>>
>>
>>
>> UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is 
>> set
>> and before DONE is set (or the last byte is shifted, I don't know 
>> excatly).
>> Refs:
>> https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
>> https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834 
>>
>>
>>
>> I found this answer/report by joan that the downstream combined support
>> isn't reliable:
>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they 
>>
>>
>>
>> My implementation differs from downstream in that I use local_irq_save()
>> to protect the polling loop. But that only protects from missing the TA
>> (downstream can miss the TA and issue a Stop).
>>
>> So currently in mainline we have a driver that says it support the 
>> standard
>> (I2C_FUNC_I2C), but it really only supports one message transfers 
>> since it
>> can't do ReStart.
>>
>> What I have done in this patch is to support ReStart for transfers with
>> 2 messages: first write, then read. But maybe a better solution is to 
>> just
>> leave this alone if it is flaky and use bitbanging instead. I don't 
>> know.
> I have not said that the approach you have taken is wrong or bad.
>

I didn't take it as such, I'm just not sure what's the best approach here,
so I added and looked up some more information

> I was only telling you that the portion inside the bcm2835_i2c_xfer:
> +    /* Combined write-read to the same address (smbus) */
> +    if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
> +        !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
> +        (msgs[0].len <= 16)) {
> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
> +
> +        return ret ? ret : 2;
> +    }
> is very specific and maybe could be done in a "generic" manner
> supporting more cases.
>

It has to be specific when it comes to number of messages. We can only
support ReStart after the first message unless we use polling for the
whole transfer. And in that case we can't disable interrupts for such
a long period and we will end up sometimes loosing Transfer Active,
resulting in Stop Condition between the messages.
So we can only do transfers with 2 messages if we want Restart.

It is possible to support more than 16 bytes for the first message,
filling the FIFO after polling TA, but I'm not sure that is common.
Mostly it's 1 or 2 bytes to set a register.
The write-read restriction isn't absolutely necessary either, but it's the
most common case I think. So it was about reusing bcm2835_i2c_xfer_msg().
A less restrictive approach would require a dedicated function I think.

> At least add a dev_warn_once for all num > 1 cases not handled by the
> code above.
>
> This gives people an opportunity to detect such a situation if they
> find something is not working as expected.
>

I agree.

After reading joan's report I wonder if it would be best to add a module
parameter like downstream has, so it can be disabled. What do you think?


Noralf.

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[kernel]

> On 20.09.2016, at 12:56, Noralf Trønnes <noralf@tronnes.org> wrote:
> 
> 
> Den 20.09.2016 12:15, skrev Martin Sperl:
>> 
>> 
>> On 20.09.2016 10:41, Noralf Trønnes wrote:
>>> 
>>> Den 20.09.2016 09:19, skrev Martin Sperl:
>>>> Hi Noralf!
>>>> 
>>>> On 19.09.2016 17:26, Noralf Trønnes wrote:
>>>>> Some SMBus protocols use Repeated Start Condition to switch from write
>>>>> mode to read mode. Devices like MMA8451 won't work without it.
>>>>> 
>>>>> When downstream implemented support for this in i2c-bcm2708, it broke
>>>>> support for some devices, so a module parameter was added and combined
>>>>> transfer was disabled by default.
>>>>> See https://github.com/raspberrypi/linux/issues/599
>>>>> It doesn't seem to have been any investigation into what the problem
>>>>> really was. Later there was added a timeout on the polling loop.
>>>>> 
>>>>> One of the devices mentioned to partially stop working was DS1307.
>>>>> 
>>>>> I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
>>>>> and AT24C32 (eeprom) in parallel without problems.
>>>>> 
>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>> ---
>>>>>  drivers/i2c/busses/i2c-bcm2835.c | 107
>>>>> +++++++++++++++++++++++++++++++++++----
>>>>>  1 file changed, 98 insertions(+), 9 deletions(-)
>>>> ...
>>>>> @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter
>>>>> *adap, struct i2c_msg msgs[],
>>>>>      int i;
>>>>>      int ret = 0;
>>>>>  +    /* Combined write-read to the same address (smbus) */
>>>>> +    if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>>>>> +        !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>>>>> +        (msgs[0].len <= 16)) {
>>>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>>>>> +
>>>>> +    return ret ? ret : 2;
>>>>> +    }
>>>>> +
>>>>>      for (i = 0; i < num; i++) {
>>>>> -        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
>>>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
>>>>>          if (ret)
>>>>>              break;
>>>>>      }
>>>> This does not seem to implement the i2c_msg api correctly.
>>>> 
>>>> As per comments in include/uapi/linux/i2c.h on line 58 only the last
>>>> message
>>>> in a group should - by default - send a STOP.
>>>> 
>>> 
>>> Apparently it's a known problem that the i2c controller doesn't support
>>> Repeated Start. It will always issue a Stop when it has transferred DLEN
>>> bytes.
>>> Refs:
>>> http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
>>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they 
>>> 
>>> 
>>> UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
>>> and before DONE is set (or the last byte is shifted, I don't know excatly).
>>> Refs:
>>> https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
>>> https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834 
>>> 
>>> 
>>> I found this answer/report by joan that the downstream combined support
>>> isn't reliable:
>>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they 
>>> 
>>> 
>>> My implementation differs from downstream in that I use local_irq_save()
>>> to protect the polling loop. But that only protects from missing the TA
>>> (downstream can miss the TA and issue a Stop).
>>> 
>>> So currently in mainline we have a driver that says it support the standard
>>> (I2C_FUNC_I2C), but it really only supports one message transfers since it
>>> can't do ReStart.
>>> 
>>> What I have done in this patch is to support ReStart for transfers with
>>> 2 messages: first write, then read. But maybe a better solution is to just
>>> leave this alone if it is flaky and use bitbanging instead. I don't know.
>> I have not said that the approach you have taken is wrong or bad.
>> 
> 
> I didn't take it as such, I'm just not sure what's the best approach here,
> so I added and looked up some more information
> 
>> I was only telling you that the portion inside the bcm2835_i2c_xfer:
>> +    /* Combined write-read to the same address (smbus) */
>> +    if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>> +        !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>> +        (msgs[0].len <= 16)) {
>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>> +
>> +        return ret ? ret : 2;
>> +    }
>> is very specific and maybe could be done in a "generic" manner
>> supporting more cases.
>> 
> 
> It has to be specific when it comes to number of messages. We can only
> support ReStart after the first message unless we use polling for the
> whole transfer. And in that case we can't disable interrupts for such
> a long period and we will end up sometimes loosing Transfer Active,
> resulting in Stop Condition between the messages.
> So we can only do transfers with 2 messages if we want Restart.
> 
> It is possible to support more than 16 bytes for the first message,
> filling the FIFO after polling TA, but I'm not sure that is common.
> Mostly it's 1 or 2 bytes to set a register.
> The write-read restriction isn't absolutely necessary either, but it's the
> most common case I think. So it was about reusing bcm2835_i2c_xfer_msg().
> A less restrictive approach would require a dedicated function I think.
> 
>> At least add a dev_warn_once for all num > 1 cases not handled by the
>> code above.
>> 
>> This gives people an opportunity to detect such a situation if they
>> find something is not working as expected.
>> 
> 
> I agree.
> 
> After reading joan's report I wonder if it would be best to add a module
> parameter like downstream has, so it can be disabled. What do you think?
> 

I guess let us start simple:
* get warning in place about always issuing a stop for num > 1
  - instead we may just want to set max_num_msgs = 1 in quirks.
* apply your patch for the write (<=16) then read case.
  - maybe by setting quirks I2C_AQ_COMB_WRITE_THEN_READ
    plus max_comb_1st_msg_len = 16 and max_num_msgs = 2

If this becomes too restrictive for some specific HW, then someone 
may want to add the missing features.

As for the module parameters: no idea if this is acceptable
or sensible.

But that’s just my 2c...

Martin

Re: [PATCH 2/3] i2c: bcm2835: Add support for combined write-read transfer

[Noralf Trønnes]

Den 20.09.2016 13:29, skrev kernel@martin.sperl.org:
>> On 20.09.2016, at 12:56, Noralf Trønnes <noralf@tronnes.org> wrote:
>>
>>
>> Den 20.09.2016 12:15, skrev Martin Sperl:
>>>
>>> On 20.09.2016 10:41, Noralf Trønnes wrote:
>>>> Den 20.09.2016 09:19, skrev Martin Sperl:
>>>>> Hi Noralf!
>>>>>
>>>>> On 19.09.2016 17:26, Noralf Trønnes wrote:
>>>>>> Some SMBus protocols use Repeated Start Condition to switch from write
>>>>>> mode to read mode. Devices like MMA8451 won't work without it.
>>>>>>
>>>>>> When downstream implemented support for this in i2c-bcm2708, it broke
>>>>>> support for some devices, so a module parameter was added and combined
>>>>>> transfer was disabled by default.
>>>>>> See https://github.com/raspberrypi/linux/issues/599
>>>>>> It doesn't seem to have been any investigation into what the problem
>>>>>> really was. Later there was added a timeout on the polling loop.
>>>>>>
>>>>>> One of the devices mentioned to partially stop working was DS1307.
>>>>>>
>>>>>> I have run thousands of transfers to a DS1307 (rtc), MMA8451 (accel)
>>>>>> and AT24C32 (eeprom) in parallel without problems.
>>>>>>
>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>> ---
>>>>>>   drivers/i2c/busses/i2c-bcm2835.c | 107
>>>>>> +++++++++++++++++++++++++++++++++++----
>>>>>>   1 file changed, 98 insertions(+), 9 deletions(-)
>>>>> ...
>>>>>> @@ -209,8 +289,17 @@ static int bcm2835_i2c_xfer(struct i2c_adapter
>>>>>> *adap, struct i2c_msg msgs[],
>>>>>>       int i;
>>>>>>       int ret = 0;
>>>>>>   +    /* Combined write-read to the same address (smbus) */
>>>>>> +    if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>>>>>> +        !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>>>>>> +        (msgs[0].len <= 16)) {
>>>>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>>>>>> +
>>>>>> +    return ret ? ret : 2;
>>>>>> +    }
>>>>>> +
>>>>>>       for (i = 0; i < num; i++) {
>>>>>> -        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
>>>>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i], NULL);
>>>>>>           if (ret)
>>>>>>               break;
>>>>>>       }
>>>>> This does not seem to implement the i2c_msg api correctly.
>>>>>
>>>>> As per comments in include/uapi/linux/i2c.h on line 58 only the last
>>>>> message
>>>>> in a group should - by default - send a STOP.
>>>>>
>>>> Apparently it's a known problem that the i2c controller doesn't support
>>>> Repeated Start. It will always issue a Stop when it has transferred DLEN
>>>> bytes.
>>>> Refs:
>>>> http://www.circuitwizard.de/raspi-i2c-fix/raspi-i2c-fix.html
>>>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>>>>
>>>>
>>>> UNLESS: a Start Transfer (ST) is issued after Transfer Active (TA) is set
>>>> and before DONE is set (or the last byte is shifted, I don't know excatly).
>>>> Refs:
>>>> https://github.com/raspberrypi/linux/issues/254#issuecomment-15254134
>>>> https://www.raspberrypi.org/forums/viewtopic.php?p=807834&sid=2b612c7209f2175bf1a266359c72ae6c#p807834
>>>>
>>>>
>>>> I found this answer/report by joan that the downstream combined support
>>>> isn't reliable:
>>>> http://raspberrypi.stackexchange.com/questions/31728/has-anyone-successfully-used-i2c-repeated-starts-on-the-pi2-my-scope-says-they
>>>>
>>>>
>>>> My implementation differs from downstream in that I use local_irq_save()
>>>> to protect the polling loop. But that only protects from missing the TA
>>>> (downstream can miss the TA and issue a Stop).
>>>>
>>>> So currently in mainline we have a driver that says it support the standard
>>>> (I2C_FUNC_I2C), but it really only supports one message transfers since it
>>>> can't do ReStart.
>>>>
>>>> What I have done in this patch is to support ReStart for transfers with
>>>> 2 messages: first write, then read. But maybe a better solution is to just
>>>> leave this alone if it is flaky and use bitbanging instead. I don't know.
>>> I have not said that the approach you have taken is wrong or bad.
>>>
>> I didn't take it as such, I'm just not sure what's the best approach here,
>> so I added and looked up some more information
>>
>>> I was only telling you that the portion inside the bcm2835_i2c_xfer:
>>> +    /* Combined write-read to the same address (smbus) */
>>> +    if (num == 2 && (msgs[0].addr == msgs[1].addr) &&
>>> +        !(msgs[0].flags & I2C_M_RD) && (msgs[1].flags & I2C_M_RD) &&
>>> +        (msgs[0].len <= 16)) {
>>> +        ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[0], &msgs[1]);
>>> +
>>> +        return ret ? ret : 2;
>>> +    }
>>> is very specific and maybe could be done in a "generic" manner
>>> supporting more cases.
>>>
>> It has to be specific when it comes to number of messages. We can only
>> support ReStart after the first message unless we use polling for the
>> whole transfer. And in that case we can't disable interrupts for such
>> a long period and we will end up sometimes loosing Transfer Active,
>> resulting in Stop Condition between the messages.
>> So we can only do transfers with 2 messages if we want Restart.
>>
>> It is possible to support more than 16 bytes for the first message,
>> filling the FIFO after polling TA, but I'm not sure that is common.
>> Mostly it's 1 or 2 bytes to set a register.
>> The write-read restriction isn't absolutely necessary either, but it's the
>> most common case I think. So it was about reusing bcm2835_i2c_xfer_msg().
>> A less restrictive approach would require a dedicated function I think.
>>
>>> At least add a dev_warn_once for all num > 1 cases not handled by the
>>> code above.
>>>
>>> This gives people an opportunity to detect such a situation if they
>>> find something is not working as expected.
>>>
>> I agree.
>>
>> After reading joan's report I wonder if it would be best to add a module
>> parameter like downstream has, so it can be disabled. What do you think?
>>
> I guess let us start simple:
> * get warning in place about always issuing a stop for num > 1
>    - instead we may just want to set max_num_msgs = 1 in quirks.
> * apply your patch for the write (<=16) then read case.
>    - maybe by setting quirks I2C_AQ_COMB_WRITE_THEN_READ
>      plus max_comb_1st_msg_len = 16 and max_num_msgs = 2
>
> If this becomes too restrictive for some specific HW, then someone
> may want to add the missing features.
>
> As for the module parameters: no idea if this is acceptable
> or sensible.
>
> But that’s just my 2c...

It suddenly struct me that I had seen the TA bit set when I debugged the
interrupt function. And it turns out that if I don't prefill the FIFO,
then I can use the TXW interrupt to know when the transfer is active,
no need for polling. It looks promising so far, need to run my testcases
to be sure.

Thanks,
Noralf.