[PATCH v3 0/7] i2c: bcm2835: Bring in changes from downstream

[PATCH v3 0/7] i2c: bcm2835: Bring in changes from downstream

[Noralf Trønnes]

This patchset tries to bring in the lessons learned in the downstream
driver i2c-bcm2708. The downstream clock stretcing timeout patch has
been left out since clock stretching is broken/unreliable on this
controller, so no point in setting it.

Changes since version 2:
- use dev_dbg instead for transfer errors
- drop i2c2 disabling patch, vc4 uses it.


Noralf.


Noralf Trønnes (7):
  i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
  i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
  i2c: bcm2835: Use dev_dbg logging on transfer errors
  i2c: bcm2835: Can't support I2C_M_IGNORE_NAK
  i2c: bcm2835: Add support for Repeated Start Condition
  i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT
  i2c: bcm2835: Add support for dynamic clock

 drivers/i2c/busses/i2c-bcm2835.c | 209 ++++++++++++++++++++++++---------------
 1 file changed, 130 insertions(+), 79 deletions(-)

--
2.8.2

[PATCH v3 0/7] i2c: bcm2835: Bring in changes from downstream

[Noralf Trønnes]

This patchset tries to bring in the lessons learned in the downstream
driver i2c-bcm2708. The downstream clock stretcing timeout patch has
been left out since clock stretching is broken/unreliable on this
controller, so no point in setting it.

Changes since version 2:
- use dev_dbg instead for transfer errors
- drop i2c2 disabling patch, vc4 uses it.


Noralf.


Noralf Tr?nnes (7):
  i2c: bcm2835: Fix hang for writing messages larger than 16 bytes
  i2c: bcm2835: Protect against unexpected TXW/RXR interrupts
  i2c: bcm2835: Use dev_dbg logging on transfer errors
  i2c: bcm2835: Can't support I2C_M_IGNORE_NAK
  i2c: bcm2835: Add support for Repeated Start Condition
  i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT
  i2c: bcm2835: Add support for dynamic clock

 drivers/i2c/busses/i2c-bcm2835.c | 209 ++++++++++++++++++++++++---------------
 1 file changed, 130 insertions(+), 79 deletions(-)

--
2.8.2

[PATCH v3 1/7] 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>
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Martin Sperl <kernel@martin.sperl.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 v3 1/7] 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>
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Martin Sperl <kernel@martin.sperl.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 v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Noralf Trønnes]

If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
the driver has no way to fill/drain the FIFO to stop the interrupts.
In this case the controller has to be disabled and the transfer
completed to avoid hang.

(CLKT | ERR) and DONE interrupts are completed in their own paths, and
the controller is disabled in the transfer function after completion.
Unite the code paths and do disabling inside the interrupt routine.

Clear interrupt status bits in the united completion path instead of
trying to do it on every interrupt which isn't necessary.
Only CLKT, ERR and DONE can be cleared that way.

Add the status value to the error value in case of TXW/RXR errors to
distinguish them from the other S_LEN error.

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

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index f283b71..df036ed 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -50,8 +50,6 @@
 #define BCM2835_I2C_S_CLKT	BIT(9)
 #define BCM2835_I2C_S_LEN	BIT(10) /* Fake bit for SW error reporting */
 
-#define BCM2835_I2C_BITMSK_S	0x03FF
-
 #define BCM2835_I2C_CDIV_MIN	0x0002
 #define BCM2835_I2C_CDIV_MAX	0xFFFE
 
@@ -117,14 +115,11 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 	u32 val, err;
 
 	val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
-	val &= BCM2835_I2C_BITMSK_S;
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, val);
 
 	err = val & (BCM2835_I2C_S_CLKT | BCM2835_I2C_S_ERR);
 	if (err) {
 		i2c_dev->msg_err = err;
-		complete(&i2c_dev->completion);
-		return IRQ_HANDLED;
+		goto complete;
 	}
 
 	if (val & BCM2835_I2C_S_DONE) {
@@ -137,21 +132,38 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 			i2c_dev->msg_err = BCM2835_I2C_S_LEN;
 		else
 			i2c_dev->msg_err = 0;
-		complete(&i2c_dev->completion);
-		return IRQ_HANDLED;
+		goto complete;
 	}
 
 	if (val & BCM2835_I2C_S_TXW) {
+		if (!i2c_dev->msg_buf_remaining) {
+			i2c_dev->msg_err = val | BCM2835_I2C_S_LEN;
+			goto complete;
+		}
+
 		bcm2835_fill_txfifo(i2c_dev);
 		return IRQ_HANDLED;
 	}
 
 	if (val & BCM2835_I2C_S_RXR) {
+		if (!i2c_dev->msg_buf_remaining) {
+			i2c_dev->msg_err = val | BCM2835_I2C_S_LEN;
+			goto complete;
+		}
+
 		bcm2835_drain_rxfifo(i2c_dev);
 		return IRQ_HANDLED;
 	}
 
 	return IRQ_NONE;
+
+complete:
+	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);
+	complete(&i2c_dev->completion);
+
+	return IRQ_HANDLED;
 }
 
 static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
@@ -181,8 +193,9 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						BCM2835_I2C_TIMEOUT);
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
 	if (!time_left) {
+		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
+				   BCM2835_I2C_C_CLEAR);
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
 		return -ETIMEDOUT;
 	}
-- 
2.8.2

[PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Noralf Trønnes]

If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
the driver has no way to fill/drain the FIFO to stop the interrupts.
In this case the controller has to be disabled and the transfer
completed to avoid hang.

(CLKT | ERR) and DONE interrupts are completed in their own paths, and
the controller is disabled in the transfer function after completion.
Unite the code paths and do disabling inside the interrupt routine.

Clear interrupt status bits in the united completion path instead of
trying to do it on every interrupt which isn't necessary.
Only CLKT, ERR and DONE can be cleared that way.

Add the status value to the error value in case of TXW/RXR errors to
distinguish them from the other S_LEN error.

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

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index f283b71..df036ed 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -50,8 +50,6 @@
 #define BCM2835_I2C_S_CLKT	BIT(9)
 #define BCM2835_I2C_S_LEN	BIT(10) /* Fake bit for SW error reporting */
 
-#define BCM2835_I2C_BITMSK_S	0x03FF
-
 #define BCM2835_I2C_CDIV_MIN	0x0002
 #define BCM2835_I2C_CDIV_MAX	0xFFFE
 
@@ -117,14 +115,11 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 	u32 val, err;
 
 	val = bcm2835_i2c_readl(i2c_dev, BCM2835_I2C_S);
-	val &= BCM2835_I2C_BITMSK_S;
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_S, val);
 
 	err = val & (BCM2835_I2C_S_CLKT | BCM2835_I2C_S_ERR);
 	if (err) {
 		i2c_dev->msg_err = err;
-		complete(&i2c_dev->completion);
-		return IRQ_HANDLED;
+		goto complete;
 	}
 
 	if (val & BCM2835_I2C_S_DONE) {
@@ -137,21 +132,38 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 			i2c_dev->msg_err = BCM2835_I2C_S_LEN;
 		else
 			i2c_dev->msg_err = 0;
-		complete(&i2c_dev->completion);
-		return IRQ_HANDLED;
+		goto complete;
 	}
 
 	if (val & BCM2835_I2C_S_TXW) {
+		if (!i2c_dev->msg_buf_remaining) {
+			i2c_dev->msg_err = val | BCM2835_I2C_S_LEN;
+			goto complete;
+		}
+
 		bcm2835_fill_txfifo(i2c_dev);
 		return IRQ_HANDLED;
 	}
 
 	if (val & BCM2835_I2C_S_RXR) {
+		if (!i2c_dev->msg_buf_remaining) {
+			i2c_dev->msg_err = val | BCM2835_I2C_S_LEN;
+			goto complete;
+		}
+
 		bcm2835_drain_rxfifo(i2c_dev);
 		return IRQ_HANDLED;
 	}
 
 	return IRQ_NONE;
+
+complete:
+	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);
+	complete(&i2c_dev->completion);
+
+	return IRQ_HANDLED;
 }
 
 static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
@@ -181,8 +193,9 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						BCM2835_I2C_TIMEOUT);
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
 	if (!time_left) {
+		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
+				   BCM2835_I2C_C_CLEAR);
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
 		return -ETIMEDOUT;
 	}
-- 
2.8.2

Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Eric Anholt]

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

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

> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
> the driver has no way to fill/drain the FIFO to stop the interrupts.
> In this case the controller has to be disabled and the transfer
> completed to avoid hang.
>
> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
> the controller is disabled in the transfer function after completion.
> Unite the code paths and do disabling inside the interrupt routine.
>
> Clear interrupt status bits in the united completion path instead of
> trying to do it on every interrupt which isn't necessary.
> Only CLKT, ERR and DONE can be cleared that way.
>
> Add the status value to the error value in case of TXW/RXR errors to
> distinguish them from the other S_LEN error.

I was surprised that not writing the TXW/RXR bits on handling their
interrupts was OK, given that we were doing so before, but it's a level
interrupt and those bits are basically ignored on write.

This patch and 3, 4, and 6 are:

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

Patch 5 is:

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

Note for future debug: The I2C_C_CLEAR on errors will take some time to
resolve -- if you were in non-idle state and I2C_C_READ, it sets an
abort_rx flag and runs through the state machine to send a NACK and a
STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
be hanging around queued up for next time we start the engine.

Patch 7 I had questions about but probably will send an ack when you
reply.

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

[PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Eric Anholt]

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

> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
> the driver has no way to fill/drain the FIFO to stop the interrupts.
> In this case the controller has to be disabled and the transfer
> completed to avoid hang.
>
> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
> the controller is disabled in the transfer function after completion.
> Unite the code paths and do disabling inside the interrupt routine.
>
> Clear interrupt status bits in the united completion path instead of
> trying to do it on every interrupt which isn't necessary.
> Only CLKT, ERR and DONE can be cleared that way.
>
> Add the status value to the error value in case of TXW/RXR errors to
> distinguish them from the other S_LEN error.

I was surprised that not writing the TXW/RXR bits on handling their
interrupts was OK, given that we were doing so before, but it's a level
interrupt and those bits are basically ignored on write.

This patch and 3, 4, and 6 are:

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

Patch 5 is:

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

Note for future debug: The I2C_C_CLEAR on errors will take some time to
resolve -- if you were in non-idle state and I2C_C_READ, it sets an
abort_rx flag and runs through the state machine to send a NACK and a
STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
be hanging around queued up for next time we start the engine.

Patch 7 I had questions about but probably will send an ack when you
reply.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160928/15c6f921/attachment.sig>

Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Eric Anholt]

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

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

> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
> the driver has no way to fill/drain the FIFO to stop the interrupts.
> In this case the controller has to be disabled and the transfer
> completed to avoid hang.
>
> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
> the controller is disabled in the transfer function after completion.
> Unite the code paths and do disabling inside the interrupt routine.
>
> Clear interrupt status bits in the united completion path instead of
> trying to do it on every interrupt which isn't necessary.
> Only CLKT, ERR and DONE can be cleared that way.
>
> Add the status value to the error value in case of TXW/RXR errors to
> distinguish them from the other S_LEN error.

I was surprised that not writing the TXW/RXR bits on handling their
interrupts was OK, given that we were doing so before, but it's a level
interrupt and those bits are basically ignored on write.

This patch and 3, 4, and 6 are:

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

Patch 5 is:

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

Note for future debug: The I2C_C_CLEAR on errors will take some time to
resolve -- if you were in non-idle state and I2C_C_READ, it sets an
abort_rx flag and runs through the state machine to send a NACK and a
STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
be hanging around queued up for next time we start the engine.

Patch 7 I had questions about but probably will send an ack when you
reply.

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

Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Noralf Trønnes]

Den 29.09.2016 00:00, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>> In this case the controller has to be disabled and the transfer
>> completed to avoid hang.
>>
>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>> the controller is disabled in the transfer function after completion.
>> Unite the code paths and do disabling inside the interrupt routine.
>>
>> Clear interrupt status bits in the united completion path instead of
>> trying to do it on every interrupt which isn't necessary.
>> Only CLKT, ERR and DONE can be cleared that way.
>>
>> Add the status value to the error value in case of TXW/RXR errors to
>> distinguish them from the other S_LEN error.
> I was surprised that not writing the TXW/RXR bits on handling their
> interrupts was OK, given that we were doing so before, but it's a level
> interrupt and those bits are basically ignored on write.
>
> This patch and 3, 4, and 6 are:
>
> Reviewed-by: Eric Anholt <eric@anholt.net>
>
> Patch 5 is:
>
> Acked-by: Eric Anholt <eric@anholt.net>
>
> Note for future debug: The I2C_C_CLEAR on errors will take some time to
> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
> abort_rx flag and runs through the state machine to send a NACK and a
> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
> be hanging around queued up for next time we start the engine.

Maybe you're able to explain the issues I had with reset:
https://github.com/raspberrypi/linux/issues/1653

Should we put your note into the commit message?
It will most likely be lost if it just stays in this email.

Noralf.

[PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Noralf Trønnes]

Den 29.09.2016 00:00, skrev Eric Anholt:
> Noralf Tr?nnes <noralf@tronnes.org> writes:
>
>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>> In this case the controller has to be disabled and the transfer
>> completed to avoid hang.
>>
>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>> the controller is disabled in the transfer function after completion.
>> Unite the code paths and do disabling inside the interrupt routine.
>>
>> Clear interrupt status bits in the united completion path instead of
>> trying to do it on every interrupt which isn't necessary.
>> Only CLKT, ERR and DONE can be cleared that way.
>>
>> Add the status value to the error value in case of TXW/RXR errors to
>> distinguish them from the other S_LEN error.
> I was surprised that not writing the TXW/RXR bits on handling their
> interrupts was OK, given that we were doing so before, but it's a level
> interrupt and those bits are basically ignored on write.
>
> This patch and 3, 4, and 6 are:
>
> Reviewed-by: Eric Anholt <eric@anholt.net>
>
> Patch 5 is:
>
> Acked-by: Eric Anholt <eric@anholt.net>
>
> Note for future debug: The I2C_C_CLEAR on errors will take some time to
> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
> abort_rx flag and runs through the state machine to send a NACK and a
> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
> be hanging around queued up for next time we start the engine.

Maybe you're able to explain the issues I had with reset:
https://github.com/raspberrypi/linux/issues/1653

Should we put your note into the commit message?
It will most likely be lost if it just stays in this email.

Noralf.

Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Stefan Wahren]

> Noralf Trønnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
> geschrieben:
> 
> 
> 
> Den 29.09.2016 00:00, skrev Eric Anholt:
> > Noralf Trønnes <noralf@tronnes.org> writes:
> >
> >> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
> >> the driver has no way to fill/drain the FIFO to stop the interrupts.
> >> In this case the controller has to be disabled and the transfer
> >> completed to avoid hang.
> >>
> >> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
> >> the controller is disabled in the transfer function after completion.
> >> Unite the code paths and do disabling inside the interrupt routine.
> >>
> >> Clear interrupt status bits in the united completion path instead of
> >> trying to do it on every interrupt which isn't necessary.
> >> Only CLKT, ERR and DONE can be cleared that way.
> >>
> >> Add the status value to the error value in case of TXW/RXR errors to
> >> distinguish them from the other S_LEN error.
> > I was surprised that not writing the TXW/RXR bits on handling their
> > interrupts was OK, given that we were doing so before, but it's a level
> > interrupt and those bits are basically ignored on write.
> >
> > This patch and 3, 4, and 6 are:
> >
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> >
> > Patch 5 is:
> >
> > Acked-by: Eric Anholt <eric@anholt.net>
> >
> > Note for future debug: The I2C_C_CLEAR on errors will take some time to
> > resolve -- if you were in non-idle state and I2C_C_READ, it sets an
> > abort_rx flag and runs through the state machine to send a NACK and a
> > STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
> > be hanging around queued up for next time we start the engine.
> 
> Maybe you're able to explain the issues I had with reset:
> https://github.com/raspberrypi/linux/issues/1653
> 
> Should we put your note into the commit message?
> It will most likely be lost if it just stays in this email.

I prefer to have this kind of information as a code comment.

> 
> Noralf.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Stefan Wahren]

> Noralf Tr?nnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
> geschrieben:
> 
> 
> 
> Den 29.09.2016 00:00, skrev Eric Anholt:
> > Noralf Tr?nnes <noralf@tronnes.org> writes:
> >
> >> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
> >> the driver has no way to fill/drain the FIFO to stop the interrupts.
> >> In this case the controller has to be disabled and the transfer
> >> completed to avoid hang.
> >>
> >> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
> >> the controller is disabled in the transfer function after completion.
> >> Unite the code paths and do disabling inside the interrupt routine.
> >>
> >> Clear interrupt status bits in the united completion path instead of
> >> trying to do it on every interrupt which isn't necessary.
> >> Only CLKT, ERR and DONE can be cleared that way.
> >>
> >> Add the status value to the error value in case of TXW/RXR errors to
> >> distinguish them from the other S_LEN error.
> > I was surprised that not writing the TXW/RXR bits on handling their
> > interrupts was OK, given that we were doing so before, but it's a level
> > interrupt and those bits are basically ignored on write.
> >
> > This patch and 3, 4, and 6 are:
> >
> > Reviewed-by: Eric Anholt <eric@anholt.net>
> >
> > Patch 5 is:
> >
> > Acked-by: Eric Anholt <eric@anholt.net>
> >
> > Note for future debug: The I2C_C_CLEAR on errors will take some time to
> > resolve -- if you were in non-idle state and I2C_C_READ, it sets an
> > abort_rx flag and runs through the state machine to send a NACK and a
> > STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
> > be hanging around queued up for next time we start the engine.
> 
> Maybe you're able to explain the issues I had with reset:
> https://github.com/raspberrypi/linux/issues/1653
> 
> Should we put your note into the commit message?
> It will most likely be lost if it just stays in this email.

I prefer to have this kind of information as a code comment.

> 
> Noralf.
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Noralf Trønnes]

Den 29.09.2016 07:37, skrev Stefan Wahren:
>> Noralf Trønnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
>> geschrieben:
>>
>>
>>
>> Den 29.09.2016 00:00, skrev Eric Anholt:
>>> Noralf Trønnes <noralf@tronnes.org> writes:
>>>
>>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>>> In this case the controller has to be disabled and the transfer
>>>> completed to avoid hang.
>>>>
>>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>>> the controller is disabled in the transfer function after completion.
>>>> Unite the code paths and do disabling inside the interrupt routine.
>>>>
>>>> Clear interrupt status bits in the united completion path instead of
>>>> trying to do it on every interrupt which isn't necessary.
>>>> Only CLKT, ERR and DONE can be cleared that way.
>>>>
>>>> Add the status value to the error value in case of TXW/RXR errors to
>>>> distinguish them from the other S_LEN error.
>>> I was surprised that not writing the TXW/RXR bits on handling their
>>> interrupts was OK, given that we were doing so before, but it's a level
>>> interrupt and those bits are basically ignored on write.
>>>
>>> This patch and 3, 4, and 6 are:
>>>
>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>
>>> Patch 5 is:
>>>
>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>
>>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>>> abort_rx flag and runs through the state machine to send a NACK and a
>>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>>> be hanging around queued up for next time we start the engine.
>> Maybe you're able to explain the issues I had with reset:
>> https://github.com/raspberrypi/linux/issues/1653
>>
>> Should we put your note into the commit message?
>> It will most likely be lost if it just stays in this email.
> I prefer to have this kind of information as a code comment.

Eric, does this look good to you as a code comment:

/*
  * Note about I2C_C_CLEAR on error:
  * The I2C_C_CLEAR on errors will take some time to resolve -- if you 
were in
  * non-idle state and I2C_C_READ, it sets an abort_rx flag and runs through
  * the state machine to send a NACK and a STOP. Since we're setting CLEAR
  * without I2CEN, that NACK will be hanging around queued up for next time
  * we start the engine.
  */


If it is, I'll resend the series with this change and add all the ack's 
and r-b's.

Noralf.

[PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Noralf Trønnes]

Den 29.09.2016 07:37, skrev Stefan Wahren:
>> Noralf Tr?nnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
>> geschrieben:
>>
>>
>>
>> Den 29.09.2016 00:00, skrev Eric Anholt:
>>> Noralf Tr?nnes <noralf@tronnes.org> writes:
>>>
>>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>>> In this case the controller has to be disabled and the transfer
>>>> completed to avoid hang.
>>>>
>>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>>> the controller is disabled in the transfer function after completion.
>>>> Unite the code paths and do disabling inside the interrupt routine.
>>>>
>>>> Clear interrupt status bits in the united completion path instead of
>>>> trying to do it on every interrupt which isn't necessary.
>>>> Only CLKT, ERR and DONE can be cleared that way.
>>>>
>>>> Add the status value to the error value in case of TXW/RXR errors to
>>>> distinguish them from the other S_LEN error.
>>> I was surprised that not writing the TXW/RXR bits on handling their
>>> interrupts was OK, given that we were doing so before, but it's a level
>>> interrupt and those bits are basically ignored on write.
>>>
>>> This patch and 3, 4, and 6 are:
>>>
>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>
>>> Patch 5 is:
>>>
>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>
>>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>>> abort_rx flag and runs through the state machine to send a NACK and a
>>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>>> be hanging around queued up for next time we start the engine.
>> Maybe you're able to explain the issues I had with reset:
>> https://github.com/raspberrypi/linux/issues/1653
>>
>> Should we put your note into the commit message?
>> It will most likely be lost if it just stays in this email.
> I prefer to have this kind of information as a code comment.

Eric, does this look good to you as a code comment:

/*
  * Note about I2C_C_CLEAR on error:
  * The I2C_C_CLEAR on errors will take some time to resolve -- if you 
were in
  * non-idle state and I2C_C_READ, it sets an abort_rx flag and runs through
  * the state machine to send a NACK and a STOP. Since we're setting CLEAR
  * without I2CEN, that NACK will be hanging around queued up for next time
  * we start the engine.
  */


If it is, I'll resend the series with this change and add all the ack's 
and r-b's.

Noralf.

Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Eric Anholt]

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

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

> Den 29.09.2016 07:37, skrev Stefan Wahren:
>>> Noralf Trønnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
>>> geschrieben:
>>>
>>>
>>>
>>> Den 29.09.2016 00:00, skrev Eric Anholt:
>>>> Noralf Trønnes <noralf@tronnes.org> writes:
>>>>
>>>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>>>> In this case the controller has to be disabled and the transfer
>>>>> completed to avoid hang.
>>>>>
>>>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>>>> the controller is disabled in the transfer function after completion.
>>>>> Unite the code paths and do disabling inside the interrupt routine.
>>>>>
>>>>> Clear interrupt status bits in the united completion path instead of
>>>>> trying to do it on every interrupt which isn't necessary.
>>>>> Only CLKT, ERR and DONE can be cleared that way.
>>>>>
>>>>> Add the status value to the error value in case of TXW/RXR errors to
>>>>> distinguish them from the other S_LEN error.
>>>> I was surprised that not writing the TXW/RXR bits on handling their
>>>> interrupts was OK, given that we were doing so before, but it's a level
>>>> interrupt and those bits are basically ignored on write.
>>>>
>>>> This patch and 3, 4, and 6 are:
>>>>
>>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>>
>>>> Patch 5 is:
>>>>
>>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>>
>>>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>>>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>>>> abort_rx flag and runs through the state machine to send a NACK and a
>>>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>>>> be hanging around queued up for next time we start the engine.
>>> Maybe you're able to explain the issues I had with reset:
>>> https://github.com/raspberrypi/linux/issues/1653
>>>
>>> Should we put your note into the commit message?
>>> It will most likely be lost if it just stays in this email.
>> I prefer to have this kind of information as a code comment.
>
> Eric, does this look good to you as a code comment:
>
> /*
>   * Note about I2C_C_CLEAR on error:
>   * The I2C_C_CLEAR on errors will take some time to resolve -- if you 
> were in
>   * non-idle state and I2C_C_READ, it sets an abort_rx flag and runs through
>   * the state machine to send a NACK and a STOP. Since we're setting CLEAR
>   * without I2CEN, that NACK will be hanging around queued up for next time
>   * we start the engine.
>   */
>
>
> If it is, I'll resend the series with this change and add all the ack's 
> and r-b's.

Looks good to me.  Thanks!

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

[PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Eric Anholt]

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

> Den 29.09.2016 07:37, skrev Stefan Wahren:
>>> Noralf Tr?nnes <noralf@tronnes.org> hat am 29. September 2016 um 00:22
>>> geschrieben:
>>>
>>>
>>>
>>> Den 29.09.2016 00:00, skrev Eric Anholt:
>>>> Noralf Tr?nnes <noralf@tronnes.org> writes:
>>>>
>>>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>>>> In this case the controller has to be disabled and the transfer
>>>>> completed to avoid hang.
>>>>>
>>>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>>>> the controller is disabled in the transfer function after completion.
>>>>> Unite the code paths and do disabling inside the interrupt routine.
>>>>>
>>>>> Clear interrupt status bits in the united completion path instead of
>>>>> trying to do it on every interrupt which isn't necessary.
>>>>> Only CLKT, ERR and DONE can be cleared that way.
>>>>>
>>>>> Add the status value to the error value in case of TXW/RXR errors to
>>>>> distinguish them from the other S_LEN error.
>>>> I was surprised that not writing the TXW/RXR bits on handling their
>>>> interrupts was OK, given that we were doing so before, but it's a level
>>>> interrupt and those bits are basically ignored on write.
>>>>
>>>> This patch and 3, 4, and 6 are:
>>>>
>>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>>
>>>> Patch 5 is:
>>>>
>>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>>
>>>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>>>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>>>> abort_rx flag and runs through the state machine to send a NACK and a
>>>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>>>> be hanging around queued up for next time we start the engine.
>>> Maybe you're able to explain the issues I had with reset:
>>> https://github.com/raspberrypi/linux/issues/1653
>>>
>>> Should we put your note into the commit message?
>>> It will most likely be lost if it just stays in this email.
>> I prefer to have this kind of information as a code comment.
>
> Eric, does this look good to you as a code comment:
>
> /*
>   * Note about I2C_C_CLEAR on error:
>   * The I2C_C_CLEAR on errors will take some time to resolve -- if you 
> were in
>   * non-idle state and I2C_C_READ, it sets an abort_rx flag and runs through
>   * the state machine to send a NACK and a STOP. Since we're setting CLEAR
>   * without I2CEN, that NACK will be hanging around queued up for next time
>   * we start the engine.
>   */
>
>
> If it is, I'll resend the series with this change and add all the ack's 
> and r-b's.

Looks good to me.  Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161003/29b87d31/attachment.sig>

Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Eric Anholt]

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

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

> Den 29.09.2016 00:00, skrev Eric Anholt:
>> Noralf Trønnes <noralf@tronnes.org> writes:
>>
>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>> In this case the controller has to be disabled and the transfer
>>> completed to avoid hang.
>>>
>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>> the controller is disabled in the transfer function after completion.
>>> Unite the code paths and do disabling inside the interrupt routine.
>>>
>>> Clear interrupt status bits in the united completion path instead of
>>> trying to do it on every interrupt which isn't necessary.
>>> Only CLKT, ERR and DONE can be cleared that way.
>>>
>>> Add the status value to the error value in case of TXW/RXR errors to
>>> distinguish them from the other S_LEN error.
>> I was surprised that not writing the TXW/RXR bits on handling their
>> interrupts was OK, given that we were doing so before, but it's a level
>> interrupt and those bits are basically ignored on write.
>>
>> This patch and 3, 4, and 6 are:
>>
>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>
>> Patch 5 is:
>>
>> Acked-by: Eric Anholt <eric@anholt.net>
>>
>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>> abort_rx flag and runs through the state machine to send a NACK and a
>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>> be hanging around queued up for next time we start the engine.
>
> Maybe you're able to explain the issues I had with reset:
> https://github.com/raspberrypi/linux/issues/1653

One of the questions I think you might have is "what state does the
controller end up in after the various interrupts?"

ERR:
- produced if we get a nack that's not at the end of a read.

- Proceeds to repeated start if BCM2835_I2C_C_ST is queued, otherwise
  stop.

CLKT:
- Triggered by a counter outside of the state machine when stretching
  happens and then times out.

- Sets cs_override, which causes proceeding through the state machine as
  if the clock wasn't getting stretched, until the end of the next byte
  sent/received.

- According to Wolfram we shouldn't be timing out on clock stretching
  for i2c, just on the transfer as a whole
  (https://patchwork.kernel.org/patch/9148431/), so I wrote
  https://github.com/anholt/linux/commit/894200276239d2e4c60b378bdc52164fcb13af8d.
  However, I don't see an obvious way to get back to IDLE while the
  slave is still stretching, without triggering the clock stretching
  timeout path.

DONE:
- Signaled at STOP, and just moves to IDLE state which keeps scl/sda
  high and waits for a BCM2835_I2C_C_ST while we're not clearing the
  FIFOs (if you do signal start while the fifos are clearing, the start
  will hang around until the fifo clear is done).  This is the only way
  to get to IDLE.

I'm don't think I have an answer to the "what should I do?" question you
had, but hopefully this helps.

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

[PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Eric Anholt]

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

> Den 29.09.2016 00:00, skrev Eric Anholt:
>> Noralf Tr?nnes <noralf@tronnes.org> writes:
>>
>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>> In this case the controller has to be disabled and the transfer
>>> completed to avoid hang.
>>>
>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>> the controller is disabled in the transfer function after completion.
>>> Unite the code paths and do disabling inside the interrupt routine.
>>>
>>> Clear interrupt status bits in the united completion path instead of
>>> trying to do it on every interrupt which isn't necessary.
>>> Only CLKT, ERR and DONE can be cleared that way.
>>>
>>> Add the status value to the error value in case of TXW/RXR errors to
>>> distinguish them from the other S_LEN error.
>> I was surprised that not writing the TXW/RXR bits on handling their
>> interrupts was OK, given that we were doing so before, but it's a level
>> interrupt and those bits are basically ignored on write.
>>
>> This patch and 3, 4, and 6 are:
>>
>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>
>> Patch 5 is:
>>
>> Acked-by: Eric Anholt <eric@anholt.net>
>>
>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>> abort_rx flag and runs through the state machine to send a NACK and a
>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>> be hanging around queued up for next time we start the engine.
>
> Maybe you're able to explain the issues I had with reset:
> https://github.com/raspberrypi/linux/issues/1653

One of the questions I think you might have is "what state does the
controller end up in after the various interrupts?"

ERR:
- produced if we get a nack that's not at the end of a read.

- Proceeds to repeated start if BCM2835_I2C_C_ST is queued, otherwise
  stop.

CLKT:
- Triggered by a counter outside of the state machine when stretching
  happens and then times out.

- Sets cs_override, which causes proceeding through the state machine as
  if the clock wasn't getting stretched, until the end of the next byte
  sent/received.

- According to Wolfram we shouldn't be timing out on clock stretching
  for i2c, just on the transfer as a whole
  (https://patchwork.kernel.org/patch/9148431/), so I wrote
  https://github.com/anholt/linux/commit/894200276239d2e4c60b378bdc52164fcb13af8d.
  However, I don't see an obvious way to get back to IDLE while the
  slave is still stretching, without triggering the clock stretching
  timeout path.

DONE:
- Signaled at STOP, and just moves to IDLE state which keeps scl/sda
  high and waits for a BCM2835_I2C_C_ST while we're not clearing the
  FIFOs (if you do signal start while the fifos are clearing, the start
  will hang around until the fifo clear is done).  This is the only way
  to get to IDLE.

I'm don't think I have an answer to the "what should I do?" question you
had, but hopefully this helps.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20161003/0b6762c3/attachment.sig>

Re: [PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Noralf Trønnes]

Den 03.10.2016 21:42, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> Den 29.09.2016 00:00, skrev Eric Anholt:
>>> Noralf Trønnes <noralf@tronnes.org> writes:
>>>
>>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>>> In this case the controller has to be disabled and the transfer
>>>> completed to avoid hang.
>>>>
>>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>>> the controller is disabled in the transfer function after completion.
>>>> Unite the code paths and do disabling inside the interrupt routine.
>>>>
>>>> Clear interrupt status bits in the united completion path instead of
>>>> trying to do it on every interrupt which isn't necessary.
>>>> Only CLKT, ERR and DONE can be cleared that way.
>>>>
>>>> Add the status value to the error value in case of TXW/RXR errors to
>>>> distinguish them from the other S_LEN error.
>>> I was surprised that not writing the TXW/RXR bits on handling their
>>> interrupts was OK, given that we were doing so before, but it's a level
>>> interrupt and those bits are basically ignored on write.
>>>
>>> This patch and 3, 4, and 6 are:
>>>
>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>
>>> Patch 5 is:
>>>
>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>
>>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>>> abort_rx flag and runs through the state machine to send a NACK and a
>>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>>> be hanging around queued up for next time we start the engine.
>> Maybe you're able to explain the issues I had with reset:
>> https://github.com/raspberrypi/linux/issues/1653
> One of the questions I think you might have is "what state does the
> controller end up in after the various interrupts?"
>
> ERR:
> - produced if we get a nack that's not at the end of a read.
>
> - Proceeds to repeated start if BCM2835_I2C_C_ST is queued, otherwise
>    stop.
>
> CLKT:
> - Triggered by a counter outside of the state machine when stretching
>    happens and then times out.
>
> - Sets cs_override, which causes proceeding through the state machine as
>    if the clock wasn't getting stretched, until the end of the next byte
>    sent/received.
>
> - According to Wolfram we shouldn't be timing out on clock stretching
>    for i2c, just on the transfer as a whole
>    (https://patchwork.kernel.org/patch/9148431/), so I wrote
>    https://github.com/anholt/linux/commit/894200276239d2e4c60b378bdc52164fcb13af8d.
>    However, I don't see an obvious way to get back to IDLE while the
>    slave is still stretching, without triggering the clock stretching
>    timeout path.

If the transfer times out, whatever the reason, we clear the fifo
(and disable). Doesn't that get us back to IDLE?

Code with my patches:

static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
                 int num)
{
[...]
     bcm2835_i2c_start_transfer(i2c_dev);

     time_left = wait_for_completion_timeout(&i2c_dev->completion,
                         adap->timeout);
     if (!time_left) {
         bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
                    BCM2835_I2C_C_CLEAR);
         dev_err(i2c_dev->dev, "i2c transfer timed out\n");
         return -ETIMEDOUT;
     }

> DONE:
> - Signaled at STOP, and just moves to IDLE state which keeps scl/sda
>    high and waits for a BCM2835_I2C_C_ST while we're not clearing the
>    FIFOs (if you do signal start while the fifos are clearing, the start
>    will hang around until the fifo clear is done).  This is the only way
>    to get to IDLE.
>
> I'm don't think I have an answer to the "what should I do?" question you
> had, but hopefully this helps.

[PATCH v3 2/7] i2c: bcm2835: Protect against unexpected TXW/RXR interrupts

[Noralf Trønnes]

Den 03.10.2016 21:42, skrev Eric Anholt:
> Noralf Tr?nnes <noralf@tronnes.org> writes:
>
>> Den 29.09.2016 00:00, skrev Eric Anholt:
>>> Noralf Tr?nnes <noralf@tronnes.org> writes:
>>>
>>>> If an unexpected TXW or RXR interrupt occurs (msg_buf_remaining == 0),
>>>> the driver has no way to fill/drain the FIFO to stop the interrupts.
>>>> In this case the controller has to be disabled and the transfer
>>>> completed to avoid hang.
>>>>
>>>> (CLKT | ERR) and DONE interrupts are completed in their own paths, and
>>>> the controller is disabled in the transfer function after completion.
>>>> Unite the code paths and do disabling inside the interrupt routine.
>>>>
>>>> Clear interrupt status bits in the united completion path instead of
>>>> trying to do it on every interrupt which isn't necessary.
>>>> Only CLKT, ERR and DONE can be cleared that way.
>>>>
>>>> Add the status value to the error value in case of TXW/RXR errors to
>>>> distinguish them from the other S_LEN error.
>>> I was surprised that not writing the TXW/RXR bits on handling their
>>> interrupts was OK, given that we were doing so before, but it's a level
>>> interrupt and those bits are basically ignored on write.
>>>
>>> This patch and 3, 4, and 6 are:
>>>
>>> Reviewed-by: Eric Anholt <eric@anholt.net>
>>>
>>> Patch 5 is:
>>>
>>> Acked-by: Eric Anholt <eric@anholt.net>
>>>
>>> Note for future debug: The I2C_C_CLEAR on errors will take some time to
>>> resolve -- if you were in non-idle state and I2C_C_READ, it sets an
>>> abort_rx flag and runs through the state machine to send a NACK and a
>>> STOP, I think.  Since we're setting CLEAR without I2CEN, that NACK will
>>> be hanging around queued up for next time we start the engine.
>> Maybe you're able to explain the issues I had with reset:
>> https://github.com/raspberrypi/linux/issues/1653
> One of the questions I think you might have is "what state does the
> controller end up in after the various interrupts?"
>
> ERR:
> - produced if we get a nack that's not at the end of a read.
>
> - Proceeds to repeated start if BCM2835_I2C_C_ST is queued, otherwise
>    stop.
>
> CLKT:
> - Triggered by a counter outside of the state machine when stretching
>    happens and then times out.
>
> - Sets cs_override, which causes proceeding through the state machine as
>    if the clock wasn't getting stretched, until the end of the next byte
>    sent/received.
>
> - According to Wolfram we shouldn't be timing out on clock stretching
>    for i2c, just on the transfer as a whole
>    (https://patchwork.kernel.org/patch/9148431/), so I wrote
>    https://github.com/anholt/linux/commit/894200276239d2e4c60b378bdc52164fcb13af8d.
>    However, I don't see an obvious way to get back to IDLE while the
>    slave is still stretching, without triggering the clock stretching
>    timeout path.

If the transfer times out, whatever the reason, we clear the fifo
(and disable). Doesn't that get us back to IDLE?

Code with my patches:

static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
                 int num)
{
[...]
     bcm2835_i2c_start_transfer(i2c_dev);

     time_left = wait_for_completion_timeout(&i2c_dev->completion,
                         adap->timeout);
     if (!time_left) {
         bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
                    BCM2835_I2C_C_CLEAR);
         dev_err(i2c_dev->dev, "i2c transfer timed out\n");
         return -ETIMEDOUT;
     }

> DONE:
> - Signaled at STOP, and just moves to IDLE state which keeps scl/sda
>    high and waits for a BCM2835_I2C_C_ST while we're not clearing the
>    FIFOs (if you do signal start while the fifos are clearing, the start
>    will hang around until the fifo clear is done).  This is the only way
>    to get to IDLE.
>
> I'm don't think I have an answer to the "what should I do?" question you
> had, but hopefully this helps.

[PATCH v3 3/7] i2c: bcm2835: Use dev_dbg 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.
Only a handful drivers use dev_err() on transfer error, so switch to
dev_dbg() instead.

Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
---

Changes:
- use dev_dbg instead of dev_err_ratelimited

 drivers/i2c/busses/i2c-bcm2835.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index df036ed..8cdb139 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -207,7 +207,7 @@ 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_dbg(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

[PATCH v3 3/7] i2c: bcm2835: Use dev_dbg 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.
Only a handful drivers use dev_err() on transfer error, so switch to
dev_dbg() instead.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
---

Changes:
- use dev_dbg instead of dev_err_ratelimited

 drivers/i2c/busses/i2c-bcm2835.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index df036ed..8cdb139 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -207,7 +207,7 @@ 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_dbg(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 v3 3/7] i2c: bcm2835: Use dev_dbg logging on transfer errors

[Martin Sperl]

On 28.09.2016, at 19:50, Noralf Trønnes <noralf@tronnes.org> wrote:
> 
> 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.
> Only a handful drivers use dev_err() on transfer error, so switch to
> dev_dbg() instead.
> 
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>

[PATCH v3 3/7] i2c: bcm2835: Use dev_dbg logging on transfer errors

[Martin Sperl]

On 28.09.2016, at 19:50, Noralf Tr?nnes <noralf@tronnes.org> wrote:
> 
> 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.
> Only a handful drivers use dev_err() on transfer error, so switch to
> dev_dbg() instead.
> 
> Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
Reviewed-by: Martin Sperl <kernel@martin.sperl.org>

[PATCH v3 4/7] i2c: bcm2835: Can't support I2C_M_IGNORE_NAK

[Noralf Trønnes]

The controller can't support this flag, so remove it.

Documentation/i2c/i2c-protocol states that all of the message is sent:

I2C_M_IGNORE_NAK:
    Normally message is interrupted immediately if there is [NA] from the
    client. Setting this flag treats any [NA] as [A], and all of
    message is sent.

>From the BCM2835 ARM Peripherals datasheet:

    The ERR field is set when the slave fails to acknowledge either
    its address or a data byte written to it.

So when the controller doesn't receive an ack, it sets ERR and raises
an interrupt. In other words, the whole message is not sent.

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

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 8cdb139..99857f8 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -203,10 +203,6 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 	if (likely(!i2c_dev->msg_err))
 		return 0;
 
-	if ((i2c_dev->msg_err & BCM2835_I2C_S_ERR) &&
-	    (msg->flags & I2C_M_IGNORE_NAK))
-		return 0;
-
 	dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
 
 	if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
-- 
2.8.2

[PATCH v3 4/7] i2c: bcm2835: Can't support I2C_M_IGNORE_NAK

[Noralf Trønnes]

The controller can't support this flag, so remove it.

Documentation/i2c/i2c-protocol states that all of the message is sent:

I2C_M_IGNORE_NAK:
    Normally message is interrupted immediately if there is [NA] from the
    client. Setting this flag treats any [NA] as [A], and all of
    message is sent.

>From the BCM2835 ARM Peripherals datasheet:

    The ERR field is set when the slave fails to acknowledge either
    its address or a data byte written to it.

So when the controller doesn't receive an ack, it sets ERR and raises
an interrupt. In other words, the whole message is not sent.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
---
 drivers/i2c/busses/i2c-bcm2835.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 8cdb139..99857f8 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -203,10 +203,6 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 	if (likely(!i2c_dev->msg_err))
 		return 0;
 
-	if ((i2c_dev->msg_err & BCM2835_I2C_S_ERR) &&
-	    (msg->flags & I2C_M_IGNORE_NAK))
-		return 0;
-
 	dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
 
 	if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
-- 
2.8.2

[PATCH v3 5/7] i2c: bcm2835: Add support for Repeated Start Condition

[Noralf Trønnes]

Documentation/i2c/i2c-protocol states that Combined transactions should
separate messages with a Start bit and end the whole transaction with a
Stop bit. This patch adds support for issuing only a Start between
messages instead of a Stop followed by a Start.

This implementation differs from downstream i2c-bcm2708 in 2 respects:
- it uses an interrupt to detect that the transfer is active instead
  of using polling. There is no interrupt for Transfer Active, but by
  not prefilling the FIFO it's possible to use the TXW interrupt.
- when resetting/disabling the controller between transfers it writes
  CLEAR to the control register instead of just zero.
  Using just zero gave many errors. This might be the reason why
  downstream had to disable this feature and make it available with a
  module parameter.

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 | 101 ++++++++++++++++++++++++---------------
 1 file changed, 63 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 99857f8..d42fee2 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -63,6 +63,7 @@ struct bcm2835_i2c_dev {
 	struct i2c_adapter adapter;
 	struct completion completion;
 	struct i2c_msg *curr_msg;
+	int num_msgs;
 	u32 msg_err;
 	u8 *msg_buf;
 	size_t msg_buf_remaining;
@@ -109,6 +110,45 @@ static void bcm2835_drain_rxfifo(struct bcm2835_i2c_dev *i2c_dev)
 	}
 }
 
+/*
+ * 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, poll the I2CS.TA flag and wait for it to be set, and then
+ * issue a read.
+ * A comment in https://github.com/raspberrypi/linux/issues/254 shows how the
+ * firmware actually does it using polling and says that it's a workaround for
+ * a problem in the state machine.
+ * It turns out that it is possible to use the TXW interrupt to know when the
+ * transfer is active, provided the FIFO has not been prefilled.
+ */
+
+static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev)
+{
+	u32 c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN;
+	struct i2c_msg *msg = i2c_dev->curr_msg;
+	bool last_msg = (i2c_dev->num_msgs == 1);
+
+	if (!i2c_dev->num_msgs)
+		return;
+
+	i2c_dev->num_msgs--;
+	i2c_dev->msg_buf = msg->buf;
+	i2c_dev->msg_buf_remaining = msg->len;
+
+	if (msg->flags & I2C_M_RD)
+		c |= BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
+	else
+		c |= BCM2835_I2C_C_INTT;
+
+	if (last_msg)
+		c |= BCM2835_I2C_C_INTD;
+
+	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);
+}
+
 static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 {
 	struct bcm2835_i2c_dev *i2c_dev = data;
@@ -142,6 +182,12 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 		}
 
 		bcm2835_fill_txfifo(i2c_dev);
+
+		if (i2c_dev->num_msgs && !i2c_dev->msg_buf_remaining) {
+			i2c_dev->curr_msg++;
+			bcm2835_i2c_start_transfer(i2c_dev);
+		}
+
 		return IRQ_HANDLED;
 	}
 
@@ -166,30 +212,25 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
-				struct i2c_msg *msg)
+static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+			    int num)
 {
-	u32 c;
+	struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	unsigned long time_left;
+	int i;
 
-	i2c_dev->curr_msg = msg;
-	i2c_dev->msg_buf = msg->buf;
-	i2c_dev->msg_buf_remaining = msg->len;
-	reinit_completion(&i2c_dev->completion);
-
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
+	for (i = 0; i < (num - 1); i++)
+		if (msgs[i].flags & I2C_M_RD) {
+			dev_warn_once(i2c_dev->dev,
+				      "only one read message supported, has to be last\n");
+			return -EOPNOTSUPP;
+		}
 
-	if (msg->flags & I2C_M_RD) {
-		c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
-	} else {
-		c = BCM2835_I2C_C_INTT;
-		bcm2835_fill_txfifo(i2c_dev);
-	}
-	c |= BCM2835_I2C_C_ST | BCM2835_I2C_C_INTD | BCM2835_I2C_C_I2CEN;
+	i2c_dev->curr_msg = msgs;
+	i2c_dev->num_msgs = num;
+	reinit_completion(&i2c_dev->completion);
 
-	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);
+	bcm2835_i2c_start_transfer(i2c_dev);
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						BCM2835_I2C_TIMEOUT);
@@ -200,31 +241,15 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 		return -ETIMEDOUT;
 	}
 
-	if (likely(!i2c_dev->msg_err))
-		return 0;
+	if (!i2c_dev->msg_err)
+		return num;
 
 	dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
 
 	if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
 		return -EREMOTEIO;
-	else
-		return -EIO;
-}
-
-static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
-			    int num)
-{
-	struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
-	int i;
-	int ret = 0;
-
-	for (i = 0; i < num; i++) {
-		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
-		if (ret)
-			break;
-	}
 
-	return ret ?: i;
+	return -EIO;
 }
 
 static u32 bcm2835_i2c_func(struct i2c_adapter *adap)
-- 
2.8.2

[PATCH v3 5/7] i2c: bcm2835: Add support for Repeated Start Condition

[Noralf Trønnes]

Documentation/i2c/i2c-protocol states that Combined transactions should
separate messages with a Start bit and end the whole transaction with a
Stop bit. This patch adds support for issuing only a Start between
messages instead of a Stop followed by a Start.

This implementation differs from downstream i2c-bcm2708 in 2 respects:
- it uses an interrupt to detect that the transfer is active instead
  of using polling. There is no interrupt for Transfer Active, but by
  not prefilling the FIFO it's possible to use the TXW interrupt.
- when resetting/disabling the controller between transfers it writes
  CLEAR to the control register instead of just zero.
  Using just zero gave many errors. This might be the reason why
  downstream had to disable this feature and make it available with a
  module parameter.

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 | 101 ++++++++++++++++++++++++---------------
 1 file changed, 63 insertions(+), 38 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 99857f8..d42fee2 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -63,6 +63,7 @@ struct bcm2835_i2c_dev {
 	struct i2c_adapter adapter;
 	struct completion completion;
 	struct i2c_msg *curr_msg;
+	int num_msgs;
 	u32 msg_err;
 	u8 *msg_buf;
 	size_t msg_buf_remaining;
@@ -109,6 +110,45 @@ static void bcm2835_drain_rxfifo(struct bcm2835_i2c_dev *i2c_dev)
 	}
 }
 
+/*
+ * 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, poll the I2CS.TA flag and wait for it to be set, and then
+ * issue a read.
+ * A comment in https://github.com/raspberrypi/linux/issues/254 shows how the
+ * firmware actually does it using polling and says that it's a workaround for
+ * a problem in the state machine.
+ * It turns out that it is possible to use the TXW interrupt to know when the
+ * transfer is active, provided the FIFO has not been prefilled.
+ */
+
+static void bcm2835_i2c_start_transfer(struct bcm2835_i2c_dev *i2c_dev)
+{
+	u32 c = BCM2835_I2C_C_ST | BCM2835_I2C_C_I2CEN;
+	struct i2c_msg *msg = i2c_dev->curr_msg;
+	bool last_msg = (i2c_dev->num_msgs == 1);
+
+	if (!i2c_dev->num_msgs)
+		return;
+
+	i2c_dev->num_msgs--;
+	i2c_dev->msg_buf = msg->buf;
+	i2c_dev->msg_buf_remaining = msg->len;
+
+	if (msg->flags & I2C_M_RD)
+		c |= BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
+	else
+		c |= BCM2835_I2C_C_INTT;
+
+	if (last_msg)
+		c |= BCM2835_I2C_C_INTD;
+
+	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);
+}
+
 static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 {
 	struct bcm2835_i2c_dev *i2c_dev = data;
@@ -142,6 +182,12 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 		}
 
 		bcm2835_fill_txfifo(i2c_dev);
+
+		if (i2c_dev->num_msgs && !i2c_dev->msg_buf_remaining) {
+			i2c_dev->curr_msg++;
+			bcm2835_i2c_start_transfer(i2c_dev);
+		}
+
 		return IRQ_HANDLED;
 	}
 
@@ -166,30 +212,25 @@ static irqreturn_t bcm2835_i2c_isr(int this_irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
-				struct i2c_msg *msg)
+static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+			    int num)
 {
-	u32 c;
+	struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	unsigned long time_left;
+	int i;
 
-	i2c_dev->curr_msg = msg;
-	i2c_dev->msg_buf = msg->buf;
-	i2c_dev->msg_buf_remaining = msg->len;
-	reinit_completion(&i2c_dev->completion);
-
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C, BCM2835_I2C_C_CLEAR);
+	for (i = 0; i < (num - 1); i++)
+		if (msgs[i].flags & I2C_M_RD) {
+			dev_warn_once(i2c_dev->dev,
+				      "only one read message supported, has to be last\n");
+			return -EOPNOTSUPP;
+		}
 
-	if (msg->flags & I2C_M_RD) {
-		c = BCM2835_I2C_C_READ | BCM2835_I2C_C_INTR;
-	} else {
-		c = BCM2835_I2C_C_INTT;
-		bcm2835_fill_txfifo(i2c_dev);
-	}
-	c |= BCM2835_I2C_C_ST | BCM2835_I2C_C_INTD | BCM2835_I2C_C_I2CEN;
+	i2c_dev->curr_msg = msgs;
+	i2c_dev->num_msgs = num;
+	reinit_completion(&i2c_dev->completion);
 
-	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);
+	bcm2835_i2c_start_transfer(i2c_dev);
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
 						BCM2835_I2C_TIMEOUT);
@@ -200,31 +241,15 @@ static int bcm2835_i2c_xfer_msg(struct bcm2835_i2c_dev *i2c_dev,
 		return -ETIMEDOUT;
 	}
 
-	if (likely(!i2c_dev->msg_err))
-		return 0;
+	if (!i2c_dev->msg_err)
+		return num;
 
 	dev_dbg(i2c_dev->dev, "i2c transfer failed: %x\n", i2c_dev->msg_err);
 
 	if (i2c_dev->msg_err & BCM2835_I2C_S_ERR)
 		return -EREMOTEIO;
-	else
-		return -EIO;
-}
-
-static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
-			    int num)
-{
-	struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
-	int i;
-	int ret = 0;
-
-	for (i = 0; i < num; i++) {
-		ret = bcm2835_i2c_xfer_msg(i2c_dev, &msgs[i]);
-		if (ret)
-			break;
-	}
 
-	return ret ?: i;
+	return -EIO;
 }
 
 static u32 bcm2835_i2c_func(struct i2c_adapter *adap)
-- 
2.8.2

[PATCH v3 6/7] i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT

[Noralf Trønnes]

Use i2c_adapter->timeout for the completion timeout value. The core
default is 1 second.

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

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index d42fee2..6a3e753 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -53,8 +53,6 @@
 #define BCM2835_I2C_CDIV_MIN	0x0002
 #define BCM2835_I2C_CDIV_MAX	0xFFFE
 
-#define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000))
-
 struct bcm2835_i2c_dev {
 	struct device *dev;
 	void __iomem *regs;
@@ -233,7 +231,7 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 	bcm2835_i2c_start_transfer(i2c_dev);
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
-						BCM2835_I2C_TIMEOUT);
+						adap->timeout);
 	if (!time_left) {
 		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
 				   BCM2835_I2C_C_CLEAR);
-- 
2.8.2

[PATCH v3 6/7] i2c: bcm2835: Support i2c-dev ioctl I2C_TIMEOUT

[Noralf Trønnes]

Use i2c_adapter->timeout for the completion timeout value. The core
default is 1 second.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
---
 drivers/i2c/busses/i2c-bcm2835.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index d42fee2..6a3e753 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -53,8 +53,6 @@
 #define BCM2835_I2C_CDIV_MIN	0x0002
 #define BCM2835_I2C_CDIV_MAX	0xFFFE
 
-#define BCM2835_I2C_TIMEOUT (msecs_to_jiffies(1000))
-
 struct bcm2835_i2c_dev {
 	struct device *dev;
 	void __iomem *regs;
@@ -233,7 +231,7 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 	bcm2835_i2c_start_transfer(i2c_dev);
 
 	time_left = wait_for_completion_timeout(&i2c_dev->completion,
-						BCM2835_I2C_TIMEOUT);
+						adap->timeout);
 	if (!time_left) {
 		bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_C,
 				   BCM2835_I2C_C_CLEAR);
-- 
2.8.2

[PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Noralf Trønnes]

Support a dynamic clock by reading the frequency and setting the
divisor in the transfer function instead of during probe.

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

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 6a3e753..2b6b682 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -58,6 +58,7 @@ struct bcm2835_i2c_dev {
 	void __iomem *regs;
 	struct clk *clk;
 	int irq;
+	u32 bus_clk_rate;
 	struct i2c_adapter adapter;
 	struct completion completion;
 	struct i2c_msg *curr_msg;
@@ -78,6 +79,30 @@ static inline u32 bcm2835_i2c_readl(struct bcm2835_i2c_dev *i2c_dev, u32 reg)
 	return readl(i2c_dev->regs + reg);
 }
 
+static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev)
+{
+	u32 divider;
+
+	divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk),
+			       i2c_dev->bus_clk_rate);
+	/*
+	 * Per the datasheet, the register is always interpreted as an even
+	 * number, by rounding down. In other words, the LSB is ignored. So,
+	 * if the LSB is set, increment the divider to avoid any issue.
+	 */
+	if (divider & 1)
+		divider++;
+	if ((divider < BCM2835_I2C_CDIV_MIN) ||
+	    (divider > BCM2835_I2C_CDIV_MAX)) {
+		dev_err_ratelimited(i2c_dev->dev, "Invalid clock-frequency\n");
+		return -EINVAL;
+	}
+
+	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider);
+
+	return 0;
+}
+
 static void bcm2835_fill_txfifo(struct bcm2835_i2c_dev *i2c_dev)
 {
 	u32 val;
@@ -215,7 +240,7 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 {
 	struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	unsigned long time_left;
-	int i;
+	int i, ret;
 
 	for (i = 0; i < (num - 1); i++)
 		if (msgs[i].flags & I2C_M_RD) {
@@ -224,6 +249,10 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 			return -EOPNOTSUPP;
 		}
 
+	ret = bcm2835_i2c_set_divider(i2c_dev);
+	if (ret)
+		return ret;
+
 	i2c_dev->curr_msg = msgs;
 	i2c_dev->num_msgs = num;
 	reinit_completion(&i2c_dev->completion);
@@ -273,7 +302,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 {
 	struct bcm2835_i2c_dev *i2c_dev;
 	struct resource *mem, *irq;
-	u32 bus_clk_rate, divider;
 	int ret;
 	struct i2c_adapter *adap;
 
@@ -297,27 +325,12 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 	}
 
 	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
-				   &bus_clk_rate);
+				   &i2c_dev->bus_clk_rate);
 	if (ret < 0) {
 		dev_warn(&pdev->dev,
 			 "Could not read clock-frequency property\n");
-		bus_clk_rate = 100000;
-	}
-
-	divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk), bus_clk_rate);
-	/*
-	 * Per the datasheet, the register is always interpreted as an even
-	 * number, by rounding down. In other words, the LSB is ignored. So,
-	 * if the LSB is set, increment the divider to avoid any issue.
-	 */
-	if (divider & 1)
-		divider++;
-	if ((divider < BCM2835_I2C_CDIV_MIN) ||
-	    (divider > BCM2835_I2C_CDIV_MAX)) {
-		dev_err(&pdev->dev, "Invalid clock-frequency\n");
-		return -ENODEV;
+		i2c_dev->bus_clk_rate = 100000;
 	}
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider);
 
 	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!irq) {
-- 
2.8.2

[PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Noralf Trønnes]

Support a dynamic clock by reading the frequency and setting the
divisor in the transfer function instead of during probe.

Signed-off-by: Noralf Tr?nnes <noralf@tronnes.org>
---
 drivers/i2c/busses/i2c-bcm2835.c | 51 +++++++++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm2835.c b/drivers/i2c/busses/i2c-bcm2835.c
index 6a3e753..2b6b682 100644
--- a/drivers/i2c/busses/i2c-bcm2835.c
+++ b/drivers/i2c/busses/i2c-bcm2835.c
@@ -58,6 +58,7 @@ struct bcm2835_i2c_dev {
 	void __iomem *regs;
 	struct clk *clk;
 	int irq;
+	u32 bus_clk_rate;
 	struct i2c_adapter adapter;
 	struct completion completion;
 	struct i2c_msg *curr_msg;
@@ -78,6 +79,30 @@ static inline u32 bcm2835_i2c_readl(struct bcm2835_i2c_dev *i2c_dev, u32 reg)
 	return readl(i2c_dev->regs + reg);
 }
 
+static int bcm2835_i2c_set_divider(struct bcm2835_i2c_dev *i2c_dev)
+{
+	u32 divider;
+
+	divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk),
+			       i2c_dev->bus_clk_rate);
+	/*
+	 * Per the datasheet, the register is always interpreted as an even
+	 * number, by rounding down. In other words, the LSB is ignored. So,
+	 * if the LSB is set, increment the divider to avoid any issue.
+	 */
+	if (divider & 1)
+		divider++;
+	if ((divider < BCM2835_I2C_CDIV_MIN) ||
+	    (divider > BCM2835_I2C_CDIV_MAX)) {
+		dev_err_ratelimited(i2c_dev->dev, "Invalid clock-frequency\n");
+		return -EINVAL;
+	}
+
+	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider);
+
+	return 0;
+}
+
 static void bcm2835_fill_txfifo(struct bcm2835_i2c_dev *i2c_dev)
 {
 	u32 val;
@@ -215,7 +240,7 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 {
 	struct bcm2835_i2c_dev *i2c_dev = i2c_get_adapdata(adap);
 	unsigned long time_left;
-	int i;
+	int i, ret;
 
 	for (i = 0; i < (num - 1); i++)
 		if (msgs[i].flags & I2C_M_RD) {
@@ -224,6 +249,10 @@ static int bcm2835_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 			return -EOPNOTSUPP;
 		}
 
+	ret = bcm2835_i2c_set_divider(i2c_dev);
+	if (ret)
+		return ret;
+
 	i2c_dev->curr_msg = msgs;
 	i2c_dev->num_msgs = num;
 	reinit_completion(&i2c_dev->completion);
@@ -273,7 +302,6 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 {
 	struct bcm2835_i2c_dev *i2c_dev;
 	struct resource *mem, *irq;
-	u32 bus_clk_rate, divider;
 	int ret;
 	struct i2c_adapter *adap;
 
@@ -297,27 +325,12 @@ static int bcm2835_i2c_probe(struct platform_device *pdev)
 	}
 
 	ret = of_property_read_u32(pdev->dev.of_node, "clock-frequency",
-				   &bus_clk_rate);
+				   &i2c_dev->bus_clk_rate);
 	if (ret < 0) {
 		dev_warn(&pdev->dev,
 			 "Could not read clock-frequency property\n");
-		bus_clk_rate = 100000;
-	}
-
-	divider = DIV_ROUND_UP(clk_get_rate(i2c_dev->clk), bus_clk_rate);
-	/*
-	 * Per the datasheet, the register is always interpreted as an even
-	 * number, by rounding down. In other words, the LSB is ignored. So,
-	 * if the LSB is set, increment the divider to avoid any issue.
-	 */
-	if (divider & 1)
-		divider++;
-	if ((divider < BCM2835_I2C_CDIV_MIN) ||
-	    (divider > BCM2835_I2C_CDIV_MAX)) {
-		dev_err(&pdev->dev, "Invalid clock-frequency\n");
-		return -ENODEV;
+		i2c_dev->bus_clk_rate = 100000;
 	}
-	bcm2835_i2c_writel(i2c_dev, BCM2835_I2C_DIV, divider);
 
 	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
 	if (!irq) {
-- 
2.8.2

Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Eric Anholt]

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

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

> Support a dynamic clock by reading the frequency and setting the
> divisor in the transfer function instead of during probe.

Is this fixing some particular case you could note in the commit
message?  As is, it makes me think that we should be using a notifier
for when the parent clock (that's the one I assume you're talking about
being dynamic) changes.  But maybe you're working around a variable VPU
clock being set by the firmware, because we don't have a notifier for
it?

I'm a bit worried because I think this is going to be pretty expensive
to be doing per transfer.

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

[PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Eric Anholt]

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

> Support a dynamic clock by reading the frequency and setting the
> divisor in the transfer function instead of during probe.

Is this fixing some particular case you could note in the commit
message?  As is, it makes me think that we should be using a notifier
for when the parent clock (that's the one I assume you're talking about
being dynamic) changes.  But maybe you're working around a variable VPU
clock being set by the firmware, because we don't have a notifier for
it?

I'm a bit worried because I think this is going to be pretty expensive
to be doing per transfer.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160928/074f1585/attachment.sig>

Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Eric Anholt]

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

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

> Support a dynamic clock by reading the frequency and setting the
> divisor in the transfer function instead of during probe.

Is this fixing some particular case you could note in the commit
message?  As is, it makes me think that we should be using a notifier
for when the parent clock (that's the one I assume you're talking about
being dynamic) changes.  But maybe you're working around a variable VPU
clock being set by the firmware, because we don't have a notifier for
it?

I'm a bit worried because I think this is going to be pretty expensive
to be doing per transfer.

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

Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Noralf Trønnes]

Den 28.09.2016 23:24, skrev Eric Anholt:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> Support a dynamic clock by reading the frequency and setting the
>> divisor in the transfer function instead of during probe.
> Is this fixing some particular case you could note in the commit
> message?  As is, it makes me think that we should be using a notifier
> for when the parent clock (that's the one I assume you're talking about
> being dynamic) changes.  But maybe you're working around a variable VPU
> clock being set by the firmware, because we don't have a notifier for
> it?
>
> I'm a bit worried because I think this is going to be pretty expensive
> to be doing per transfer.

Actually I thought that this was a known problem, but trying to track
it down now, I can't find anything definitive. I don't have any tool
to look at the bus signals, so I couldn't actually test it.
Sorry about this, we can just drop this patch.

Noralf.

[PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Noralf Trønnes]

Den 28.09.2016 23:24, skrev Eric Anholt:
> Noralf Tr?nnes <noralf@tronnes.org> writes:
>
>> Support a dynamic clock by reading the frequency and setting the
>> divisor in the transfer function instead of during probe.
> Is this fixing some particular case you could note in the commit
> message?  As is, it makes me think that we should be using a notifier
> for when the parent clock (that's the one I assume you're talking about
> being dynamic) changes.  But maybe you're working around a variable VPU
> clock being set by the firmware, because we don't have a notifier for
> it?
>
> I'm a bit worried because I think this is going to be pretty expensive
> to be doing per transfer.

Actually I thought that this was a known problem, but trying to track
it down now, I can't find anything definitive. I don't have any tool
to look at the bus signals, so I couldn't actually test it.
Sorry about this, we can just drop this patch.

Noralf.

Re: [PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Martin Sperl]

On 28.09.2016 23:24, Eric Anholt wrote:
> Noralf Trønnes <noralf@tronnes.org> writes:
>
>> Support a dynamic clock by reading the frequency and setting the
>> divisor in the transfer function instead of during probe.
> Is this fixing some particular case you could note in the commit
> message?  As is, it makes me think that we should be using a notifier
> for when the parent clock (that's the one I assume you're talking about
> being dynamic) changes.  But maybe you're working around a variable VPU
> clock being set by the firmware, because we don't have a notifier for
> it?
>
> I'm a bit worried because I think this is going to be pretty expensive
> to be doing per transfer.

Well, the clocks are all configured without CLK_GET_RATE_NOCACHE et. al.,
so the value is read from cache, which is not (very) expensive
(see clk_core_get_rate).

This also means that any clock change of the VPU done by the firmware
does not propagate to the linux kernel anyway and the unchanged
cached value is returned.

To make this work it would require a notification mechanism from the
firmware to trigger a re-validation of all the caches. (or some sort of 
watchdog
process).

Adding a notifier to each driver (I2C, SPI) instead is - imo - a lot of 
unnecessary
code complexity, as any currently running transfer would still be impacted,
because changing the clock-divider in flight is a asking for trouble.
But then changing the vpu-clock speed while a I2s/SPI/... transfer is 
running is
also asking for trouble....

The only place where - IMO - a notifier would make sense is with the 
auxiliar
UART driver(8250_bcm2835aux.c), as there we only read the clock rates
when setting/changing the baud rate. But - again -  this would require some
notification by the firmware in the first place and any reception in the
window of change would go wrong because of unexpected effective baud
rate changes.

So as far as I can tell the change to read the current clock rate in the
transfer function is a reasonable approach and the clock framework should
handle the communication with the firmware about such changes.
(And I remember having had some discussions around this subject
with Phil Elwell or popcornmix some time ago on github where it boiled
down to: what is the "right" interface? - I can not find the reference
right now)

Reviewed-by: Martin Sperl <kernel@martin.sperl.org>

Thanks, Martin

[PATCH v3 7/7] i2c: bcm2835: Add support for dynamic clock

[Martin Sperl]

On 28.09.2016 23:24, Eric Anholt wrote:
> Noralf Tr?nnes <noralf@tronnes.org> writes:
>
>> Support a dynamic clock by reading the frequency and setting the
>> divisor in the transfer function instead of during probe.
> Is this fixing some particular case you could note in the commit
> message?  As is, it makes me think that we should be using a notifier
> for when the parent clock (that's the one I assume you're talking about
> being dynamic) changes.  But maybe you're working around a variable VPU
> clock being set by the firmware, because we don't have a notifier for
> it?
>
> I'm a bit worried because I think this is going to be pretty expensive
> to be doing per transfer.

Well, the clocks are all configured without CLK_GET_RATE_NOCACHE et. al.,
so the value is read from cache, which is not (very) expensive
(see clk_core_get_rate).

This also means that any clock change of the VPU done by the firmware
does not propagate to the linux kernel anyway and the unchanged
cached value is returned.

To make this work it would require a notification mechanism from the
firmware to trigger a re-validation of all the caches. (or some sort of 
watchdog
process).

Adding a notifier to each driver (I2C, SPI) instead is - imo - a lot of 
unnecessary
code complexity, as any currently running transfer would still be impacted,
because changing the clock-divider in flight is a asking for trouble.
But then changing the vpu-clock speed while a I2s/SPI/... transfer is 
running is
also asking for trouble....

The only place where - IMO - a notifier would make sense is with the 
auxiliar
UART driver(8250_bcm2835aux.c), as there we only read the clock rates
when setting/changing the baud rate. But - again -  this would require some
notification by the firmware in the first place and any reception in the
window of change would go wrong because of unexpected effective baud
rate changes.

So as far as I can tell the change to read the current clock rate in the
transfer function is a reasonable approach and the clock framework should
handle the communication with the firmware about such changes.
(And I remember having had some discussions around this subject
with Phil Elwell or popcornmix some time ago on github where it boiled
down to: what is the "right" interface? - I can not find the reference
right now)

Reviewed-by: Martin Sperl <kernel@martin.sperl.org>

Thanks, Martin