[PATCH 0/7] i2c: img-scb: enchancements to support i2c on pistachio

[PATCH 0/7] i2c: img-scb: enchancements to support i2c on pistachio

[Sifan Naeem]

Following patches are required to enchance the existing driver to
support i2c on pistachio.

This patch series depends on the series of fixes posted earlier[1].
The features added in this series were tested with the earlier fixes
series.

Tested on Pistachio bub and on tz1090 using an Adafruit I2C
Non-Volatile FRAM Breakout (256Kbit / 32KByte) eeprom.

Used i2c buildroot tools to test the eeprom and the other i2c blocks.
Also used dd commands to copy data to and then to dump data from the
eeprom. i2ctransfer was used to test repeated starts and verified
using a scope.

[1]:
http://marc.info/?l=linux-i2c&m=143799753022541&w=2

Sifan Naeem (7):
  i2c: img-scb: support I2C_M_IGNORE_NAK
  i2c: img-scb: support repeated starts on IP v3.3
  i2c: img-scb: mark transaction as complete when all data is read
  i2c: img-scb: mark transaction as complete when no more data to write
  i2c: img-scb: remove fifo EMPTYING interrupts handle
  i2c: img-scb: add handle for stop detected interrupt
  i2c: img-scb: add handle for Master halt interrupt

 drivers/i2c/busses/i2c-img-scb.c |  129 ++++++++++++++++++++++++++++++--------
 1 file changed, 104 insertions(+), 25 deletions(-)

-- 
1.7.9.5

[PATCH 1/7] i2c: img-scb: support I2C_M_IGNORE_NAK

[Sifan Naeem]

This commit adds support for the I2C_M_IGNORE_NAK protocol
modification.

Such behaviour can only be implemented in atomic mode. So, if a
transaction contains a message with such flag the drivers
switches to atomic mode. The implementation consists simply in
treating NAKs as ACKs.

Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 07a039c..31cd8c3 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -767,7 +767,9 @@ static unsigned int img_i2c_atomic(struct img_i2c *i2c,
 			next_cmd = CMD_RET_ACK;
 		break;
 	case CMD_RET_ACK:
-		if (i2c->line_status & LINESTAT_ACK_DET) {
+		if (i2c->line_status & LINESTAT_ACK_DET ||
+		    (i2c->line_status & LINESTAT_NACK_DET &&
+		    i2c->msg.flags & I2C_M_IGNORE_NAK)) {
 			if (i2c->msg.len == 0) {
 				next_cmd = CMD_GEN_STOP;
 			} else if (i2c->msg.flags & I2C_M_RD) {
@@ -1042,20 +1044,23 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		return -EIO;
 
 	for (i = 0; i < num; i++) {
-		if (likely(msgs[i].len))
-			continue;
 		/*
 		 * 0 byte reads are not possible because the slave could try
 		 * and pull the data line low, preventing a stop bit.
 		 */
-		if (unlikely(msgs[i].flags & I2C_M_RD))
+		if (!msgs[i].len && msgs[i].flags & I2C_M_RD)
 			return -EIO;
 		/*
 		 * 0 byte writes are possible and used for probing, but we
 		 * cannot do them in automatic mode, so use atomic mode
 		 * instead.
+		 *
+		 * Also, the I2C_M_IGNORE_NAK mode can only be implemented
+		 * in atomic mode.
 		 */
-		atomic = true;
+		if (!msgs[i].len ||
+		    (msgs[i].flags & I2C_M_IGNORE_NAK))
+			atomic = true;
 	}
 
 	ret = clk_prepare_enable(i2c->scb_clk);
-- 
1.7.9.5

[PATCH 2/7] i2c: img-scb: support repeated starts on IP v3.3

[Sifan Naeem]

In version 3.3 of the IP when transaction halt is set, an interrupt
will be generated after each byte of a transfer instead of after
every transfer but before the stop bit.
Due to this behaviour we have to be careful that every time we
release the transaction halt we have to re-enable it straight away
so that we only process a single byte, not doing so will result in
all remaining bytes been processed and a stop bit being issued,
which will prevent us having a repeated start.

This change will have no effect on earlier versions of the IP.

Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   43 +++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 31cd8c3..24b09fe 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -525,7 +525,17 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
 	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
 }
 
-/* enable or release transaction halt for control of repeated starts */
+/*
+ * Enable or release transaction halt for control of repeated starts.
+ * In version 3.3 of the IP when transaction halt is set, an interrupt
+ * will be generated after each byte of a transfer instead of after
+ * every transfer but before the stop bit.
+ * Due to this behaviour we have to be careful that every time we
+ * release the transaction halt we have to re-enable it straight away
+ * so that we only process a single byte, not doing so will result in
+ * all remaining bytes been processed and a stop bit being issued,
+ * which will prevent us having a repeated start.
+ */
 static void img_i2c_transaction_halt(struct img_i2c *i2c, bool t_halt)
 {
 	u32 val;
@@ -594,7 +604,6 @@ static void img_i2c_read(struct img_i2c *i2c)
 	img_i2c_writel(i2c, SCB_READ_ADDR_REG, i2c->msg.addr);
 	img_i2c_writel(i2c, SCB_READ_COUNT_REG, i2c->msg.len);
 
-	img_i2c_transaction_halt(i2c, false);
 	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
 }
 
@@ -608,7 +617,6 @@ static void img_i2c_write(struct img_i2c *i2c)
 	img_i2c_writel(i2c, SCB_WRITE_ADDR_REG, i2c->msg.addr);
 	img_i2c_writel(i2c, SCB_WRITE_COUNT_REG, i2c->msg.len);
 
-	img_i2c_transaction_halt(i2c, false);
 	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
 	img_i2c_write_fifo(i2c);
 
@@ -1090,12 +1098,31 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		i2c->last_msg = (i == num - 1);
 		reinit_completion(&i2c->msg_complete);
 
-		if (atomic)
+		if (atomic) {
 			img_i2c_atomic_start(i2c);
-		else if (msg->flags & I2C_M_RD)
-			img_i2c_read(i2c);
-		else
-			img_i2c_write(i2c);
+		} else {
+			/*
+			 * Enable transaction halt if not the last message in
+			 * the queue so that we can control repeated starts.
+			 */
+			img_i2c_transaction_halt(i2c, !i2c->last_msg);
+
+			if (msg->flags & I2C_M_RD)
+				img_i2c_read(i2c);
+			else
+				img_i2c_write(i2c);
+
+			/*
+			 * Release and then enable transaction halt, to
+			 * allow only a single byte to proceed.
+			 * This doesn't have an effect on the initial transfer
+			 * but will allow the following transfers to start
+			 * processing if the previous transfer was marked as
+			 * complete while the i2c block was halted.
+			 */
+			img_i2c_transaction_halt(i2c, false);
+			img_i2c_transaction_halt(i2c, !i2c->last_msg);
+		}
 		spin_unlock_irqrestore(&i2c->lock, flags);
 
 		time_left = wait_for_completion_timeout(&i2c->msg_complete,
-- 
1.7.9.5

[PATCH 3/7] i2c: img-scb: mark transaction as complete when all data is read

[Sifan Naeem]

We can mark the transfer as complete without waiting for the stop
bit. This is important when handling repeated start transfers as
we have to start the next transfer without the stop bit being issued.

Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 24b09fe..e27c3e0 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -903,8 +903,11 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 	if (i2c->msg.flags & I2C_M_RD) {
 		if (int_status & INT_FIFO_FULL_FILLING) {
 			img_i2c_read_fifo(i2c);
-			if (i2c->msg.len == 0)
-				return ISR_WAITSTOP;
+			if (i2c->msg.len == 0) {
+				if (i2c->last_msg)
+					return ISR_WAITSTOP;
+				return ISR_COMPLETE(0);
+			}
 		}
 	} else {
 		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
-- 
1.7.9.5

[PATCH 4/7] i2c: img-scb: mark transaction as complete when no more data to write

[Sifan Naeem]

We can mark the transfer as complete without waiting for the stop
bit. This is important when handling repeated start transfers as
we have to start the next transfer without the stop bit being issued.

This doesn't affect the older versions of the IP.

Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index e27c3e0..efad4d7 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -911,16 +911,11 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 		}
 	} else {
 		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
-			/*
-			 * The write fifo empty indicates that we're in the
-			 * last byte so it's safe to start a new write
-			 * transaction without losing any bytes from the
-			 * previous one.
-			 * see 2.3.7 Repeated Start Transactions.
-			 */
-			if ((int_status & INT_FIFO_EMPTY) &&
-			    i2c->msg.len == 0)
-				return ISR_WAITSTOP;
+			if (i2c->msg.len == 0) {
+				if (i2c->last_msg)
+					return ISR_WAITSTOP;
+				return ISR_COMPLETE(0);
+			}
 			img_i2c_write_fifo(i2c);
 		}
 	}
-- 
1.7.9.5

[PATCH 5/7] i2c: img-scb: remove fifo EMPTYING interrupts handle

[Sifan Naeem]

This interrupt could have been useful for repeated start transfers
as the current transfer could be marked as complete while it's
processing the final byte of the transfer.
But having to use the transaction halt interrupt to safely control
repeated start transfers, means handling of the fifo EMPTYING
interrupts is no longer necessary.

Handling this interrupt along with Transaction Halt interrupt can
cause erratic behaviour.

Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index efad4d7..10141a9 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -154,7 +154,6 @@
 #define INT_TIMING			BIT(18)
 
 #define INT_FIFO_FULL_FILLING	(INT_FIFO_FULL  | INT_FIFO_FILLING)
-#define INT_FIFO_EMPTY_EMPTYING	(INT_FIFO_EMPTY | INT_FIFO_EMPTYING)
 
 /* Level interrupts need clearing after handling instead of before */
 #define INT_LEVEL			0x01e00
@@ -176,8 +175,7 @@
 					 INT_WRITE_ACK_ERR    | \
 					 INT_FIFO_FULL        | \
 					 INT_FIFO_FILLING     | \
-					 INT_FIFO_EMPTY       | \
-					 INT_FIFO_EMPTYING)
+					 INT_FIFO_EMPTY)
 
 #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
 					 INT_ADDR_ACK_ERR     | \
@@ -910,7 +908,7 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 			}
 		}
 	} else {
-		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
+		if (int_status & INT_FIFO_EMPTY) {
 			if (i2c->msg.len == 0) {
 				if (i2c->last_msg)
 					return ISR_WAITSTOP;
-- 
1.7.9.5

[PATCH 6/7] i2c: img-scb: add handle for stop detected interrupt

[Sifan Naeem]

Stop Detected interrupt is triggered when a Stop bit is detected on
the bus, which indicates the end of the current transfer.

When the end of a transfer is indicated by the Stop bit interrupt,
drain the FIFO and signal completion for the transaction. But if the
interrupt was triggered before all data is written to the fifo or with
more data expected return error with transfer complete signal.

Halting the bus is no longer necessary after a stop bit is detected
on the bus, as there cannot be a repeated start transfer when the stop
bit has been issued, hence remove the transaction halt bit.

Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 10141a9..90faf48 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -152,6 +152,7 @@
 #define INT_TRANSACTION_DONE		BIT(15)
 #define INT_SLAVE_EVENT			BIT(16)
 #define INT_TIMING			BIT(18)
+#define INT_STOP_DETECTED		BIT(19)
 
 #define INT_FIFO_FULL_FILLING	(INT_FIFO_FULL  | INT_FIFO_FILLING)
 
@@ -175,7 +176,8 @@
 					 INT_WRITE_ACK_ERR    | \
 					 INT_FIFO_FULL        | \
 					 INT_FIFO_FILLING     | \
-					 INT_FIFO_EMPTY)
+					 INT_FIFO_EMPTY       | \
+					 INT_STOP_DETECTED)
 
 #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
 					 INT_ADDR_ACK_ERR     | \
@@ -907,6 +909,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 				return ISR_COMPLETE(0);
 			}
 		}
+		if (int_status & INT_STOP_DETECTED) {
+			int ret;
+			/*
+			 * Stop bit indicates the end of the transfer, it means
+			 * we should read all the data (or drain the FIFO). We
+			 * must signal completion for this transaction.
+			 */
+			img_i2c_transaction_halt(i2c, false);
+			img_i2c_read_fifo(i2c);
+			ret = (i2c->msg.len == 0) ? 0 : EIO;
+			return ISR_COMPLETE(ret);
+		}
 	} else {
 		if (int_status & INT_FIFO_EMPTY) {
 			if (i2c->msg.len == 0) {
@@ -916,6 +930,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 			}
 			img_i2c_write_fifo(i2c);
 		}
+		if (int_status & INT_STOP_DETECTED) {
+			int ret;
+
+			img_i2c_transaction_halt(i2c, false);
+			/*
+			 * Stop bit indicates the end of a transfer and if the
+			 * transfer has ended before all data is written to the
+			 * fifo, return an error with transfer complete signal.
+			 */
+			ret = (i2c->msg.len == 0) ? 0 : EIO;
+			return ISR_COMPLETE(ret);
+		}
 	}
 
 	return 0;
-- 
1.7.9.5

[PATCH 7/7] i2c: img-scb: add handle for Master halt interrupt

[Sifan Naeem]

Master halt is issued after each byte of a transaction is processed in
IP version 3.3.
Master halt will stall the bus by holding the SCK line low until the
halt bit in the scb_general_control is cleared.

After the last byte of a transfer is processed we can use the Master
Halt interrupt to facilitate a repeated start transfer without
issuing a stop bit.

Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 90faf48..df3d25a 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -151,6 +151,7 @@
 #define INT_FIFO_EMPTYING		BIT(12)
 #define INT_TRANSACTION_DONE		BIT(15)
 #define INT_SLAVE_EVENT			BIT(16)
+#define INT_MASTER_HALTED		BIT(17)
 #define INT_TIMING			BIT(18)
 #define INT_STOP_DETECTED		BIT(19)
 
@@ -177,6 +178,7 @@
 					 INT_FIFO_FULL        | \
 					 INT_FIFO_FILLING     | \
 					 INT_FIFO_EMPTY       | \
+					 INT_MASTER_HALTED    | \
 					 INT_STOP_DETECTED)
 
 #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
@@ -901,6 +903,17 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
 
 	if (i2c->msg.flags & I2C_M_RD) {
+		if (int_status & INT_MASTER_HALTED) {
+			img_i2c_read_fifo(i2c);
+			if (i2c->msg.len == 0)
+				return ISR_COMPLETE(0);
+			/*
+			 * Release and then enable transaction halt, to
+			 * allow only a single byte to proceed.
+			 */
+			img_i2c_transaction_halt(i2c, false);
+			img_i2c_transaction_halt(i2c, !i2c->last_msg);
+		}
 		if (int_status & INT_FIFO_FULL_FILLING) {
 			img_i2c_read_fifo(i2c);
 			if (i2c->msg.len == 0) {
@@ -922,6 +935,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 			return ISR_COMPLETE(ret);
 		}
 	} else {
+		if (int_status & INT_MASTER_HALTED) {
+			if ((int_status & INT_FIFO_EMPTY) &&
+			    i2c->msg.len == 0)
+				return ISR_COMPLETE(0);
+			img_i2c_write_fifo(i2c);
+			/*
+			 * Release and then enable transaction halt, to
+			 * allow only a single byte to proceed.
+			 */
+			img_i2c_transaction_halt(i2c, false);
+			img_i2c_transaction_halt(i2c, !i2c->last_msg);
+		}
 		if (int_status & INT_FIFO_EMPTY) {
 			if (i2c->msg.len == 0) {
 				if (i2c->last_msg)
-- 
1.7.9.5

Re: [PATCH 1/7] i2c: img-scb: support I2C_M_IGNORE_NAK

[James Hogan]

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

On 27/07/15 12:55, Sifan Naeem wrote:
> This commit adds support for the I2C_M_IGNORE_NAK protocol
> modification.
> 
> Such behaviour can only be implemented in atomic mode. So, if a
> transaction contains a message with such flag the drivers
> switches to atomic mode. The implementation consists simply in
> treating NAKs as ACKs.
> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Looks good,

Acked-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>

Thanks!
James

> ---
>  drivers/i2c/busses/i2c-img-scb.c |   15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 07a039c..31cd8c3 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -767,7 +767,9 @@ static unsigned int img_i2c_atomic(struct img_i2c *i2c,
>  			next_cmd = CMD_RET_ACK;
>  		break;
>  	case CMD_RET_ACK:
> -		if (i2c->line_status & LINESTAT_ACK_DET) {
> +		if (i2c->line_status & LINESTAT_ACK_DET ||
> +		    (i2c->line_status & LINESTAT_NACK_DET &&
> +		    i2c->msg.flags & I2C_M_IGNORE_NAK)) {
>  			if (i2c->msg.len == 0) {
>  				next_cmd = CMD_GEN_STOP;
>  			} else if (i2c->msg.flags & I2C_M_RD) {
> @@ -1042,20 +1044,23 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  		return -EIO;
>  
>  	for (i = 0; i < num; i++) {
> -		if (likely(msgs[i].len))
> -			continue;
>  		/*
>  		 * 0 byte reads are not possible because the slave could try
>  		 * and pull the data line low, preventing a stop bit.
>  		 */
> -		if (unlikely(msgs[i].flags & I2C_M_RD))
> +		if (!msgs[i].len && msgs[i].flags & I2C_M_RD)
>  			return -EIO;
>  		/*
>  		 * 0 byte writes are possible and used for probing, but we
>  		 * cannot do them in automatic mode, so use atomic mode
>  		 * instead.
> +		 *
> +		 * Also, the I2C_M_IGNORE_NAK mode can only be implemented
> +		 * in atomic mode.
>  		 */
> -		atomic = true;
> +		if (!msgs[i].len ||
> +		    (msgs[i].flags & I2C_M_IGNORE_NAK))
> +			atomic = true;
>  	}
>  
>  	ret = clk_prepare_enable(i2c->scb_clk);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/7] i2c: img-scb: support repeated starts on IP v3.3

[James Hogan]

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

Hi Sifan,

On 27/07/15 12:55, Sifan Naeem wrote:
> In version 3.3 of the IP when transaction halt is set, an interrupt
> will be generated after each byte of a transfer instead of after
> every transfer but before the stop bit.
> Due to this behaviour we have to be careful that every time we
> release the transaction halt we have to re-enable it straight away
> so that we only process a single byte, not doing so will result in
> all remaining bytes been processed and a stop bit being issued,
> which will prevent us having a repeated start.
> 
> This change will have no effect on earlier versions of the IP.

Do we still need the img_i2c_transaction_halt() in img_i2c_auto, or the
i2c->int_enable |= INT_SLAVE_EVENT in img_i2c_read/img_i2c_write now if
we're not bothering to wait until the start bit is detected before
re-enabling transaction halt?

> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   43 +++++++++++++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 31cd8c3..24b09fe 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -525,7 +525,17 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
>  	img_i2c_writel(i2c, SCB_INT_MASK_REG, i2c->int_enable);
>  }
>  
> -/* enable or release transaction halt for control of repeated starts */
> +/*
> + * Enable or release transaction halt for control of repeated starts.
> + * In version 3.3 of the IP when transaction halt is set, an interrupt
> + * will be generated after each byte of a transfer instead of after
> + * every transfer but before the stop bit.
> + * Due to this behaviour we have to be careful that every time we
> + * release the transaction halt we have to re-enable it straight away
> + * so that we only process a single byte, not doing so will result in
> + * all remaining bytes been processed and a stop bit being issued,
> + * which will prevent us having a repeated start.
> + */
>  static void img_i2c_transaction_halt(struct img_i2c *i2c, bool t_halt)
>  {
>  	u32 val;
> @@ -594,7 +604,6 @@ static void img_i2c_read(struct img_i2c *i2c)
>  	img_i2c_writel(i2c, SCB_READ_ADDR_REG, i2c->msg.addr);
>  	img_i2c_writel(i2c, SCB_READ_COUNT_REG, i2c->msg.len);
>  
> -	img_i2c_transaction_halt(i2c, false);
>  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
>  }
>  
> @@ -608,7 +617,6 @@ static void img_i2c_write(struct img_i2c *i2c)
>  	img_i2c_writel(i2c, SCB_WRITE_ADDR_REG, i2c->msg.addr);
>  	img_i2c_writel(i2c, SCB_WRITE_COUNT_REG, i2c->msg.len);
>  
> -	img_i2c_transaction_halt(i2c, false);
>  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
>  	img_i2c_write_fifo(i2c);
>  
> @@ -1090,12 +1098,31 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  		i2c->last_msg = (i == num - 1);
>  		reinit_completion(&i2c->msg_complete);
>  
> -		if (atomic)
> +		if (atomic) {
>  			img_i2c_atomic_start(i2c);
> -		else if (msg->flags & I2C_M_RD)
> -			img_i2c_read(i2c);
> -		else
> -			img_i2c_write(i2c);
> +		} else {
> +			/*
> +			 * Enable transaction halt if not the last message in
> +			 * the queue so that we can control repeated starts.
> +			 */
> +			img_i2c_transaction_halt(i2c, !i2c->last_msg);
> +
> +			if (msg->flags & I2C_M_RD)
> +				img_i2c_read(i2c);
> +			else
> +				img_i2c_write(i2c);
> +
> +			/*
> +			 * Release and then enable transaction halt, to
> +			 * allow only a single byte to proceed.
> +			 * This doesn't have an effect on the initial transfer
> +			 * but will allow the following transfers to start
> +			 * processing if the previous transfer was marked as
> +			 * complete while the i2c block was halted.
> +			 */
> +			img_i2c_transaction_halt(i2c, false);
> +			img_i2c_transaction_halt(i2c, !i2c->last_msg);

Do we have confirmation from the hw guys that no matter how fast these
two functions execute, the two writes will be sufficient to allow the
transaction to continue?

Should something also be doing this quick temporary disable of t_halt
when the interrupt is generated after each byte? I don't see anything
doing that at the moment?

Cheers
James

> +		}
>  		spin_unlock_irqrestore(&i2c->lock, flags);
>  
>  		time_left = wait_for_completion_timeout(&i2c->msg_complete,
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/7] i2c: img-scb: mark transaction as complete when all data is read

[James Hogan]

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

On 27/07/15 12:55, Sifan Naeem wrote:
> We can mark the transfer as complete without waiting for the stop
> bit. This is important when handling repeated start transfers as
> we have to start the next transfer without the stop bit being issued.
> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 24b09fe..e27c3e0 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -903,8 +903,11 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  	if (i2c->msg.flags & I2C_M_RD) {
>  		if (int_status & INT_FIFO_FULL_FILLING) {
>  			img_i2c_read_fifo(i2c);
> -			if (i2c->msg.len == 0)
> -				return ISR_WAITSTOP;
> +			if (i2c->msg.len == 0) {
> +				if (i2c->last_msg)
> +					return ISR_WAITSTOP;
> +				return ISR_COMPLETE(0);

That already happens in img_i2c_isr().

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/7] i2c: img-scb: mark transaction as complete when no more data to write

[James Hogan]

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

On 27/07/15 12:55, Sifan Naeem wrote:
> We can mark the transfer as complete without waiting for the stop
> bit. This is important when handling repeated start transfers as
> we have to start the next transfer without the stop bit being issued.
> 
> This doesn't affect the older versions of the IP.
> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index e27c3e0..efad4d7 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -911,16 +911,11 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  		}
>  	} else {
>  		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
> -			/*
> -			 * The write fifo empty indicates that we're in the
> -			 * last byte so it's safe to start a new write
> -			 * transaction without losing any bytes from the
> -			 * previous one.
> -			 * see 2.3.7 Repeated Start Transactions.
> -			 */
> -			if ((int_status & INT_FIFO_EMPTY) &&
> -			    i2c->msg.len == 0)
> -				return ISR_WAITSTOP;
> +			if (i2c->msg.len == 0) {
> +				if (i2c->last_msg)
> +					return ISR_WAITSTOP;
> +				return ISR_COMPLETE(0);
> +			}

Again, already happens in img_i2c_isr().

Cheers
James

>  			img_i2c_write_fifo(i2c);
>  		}
>  	}
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 3/7] i2c: img-scb: mark transaction as complete when all data is read

[Sifan Naeem]

Hi James,

> 
> On 27/07/15 12:55, Sifan Naeem wrote:
> > We can mark the transfer as complete without waiting for the stop bit.
> > This is important when handling repeated start transfers as we have to
> > start the next transfer without the stop bit being issued.
> >
> > Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |    7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index 24b09fe..e27c3e0 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -903,8 +903,11 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> >  	if (i2c->msg.flags & I2C_M_RD) {
> >  		if (int_status & INT_FIFO_FULL_FILLING) {
> >  			img_i2c_read_fifo(i2c);
> > -			if (i2c->msg.len == 0)
> > -				return ISR_WAITSTOP;
> > +			if (i2c->msg.len == 0) {
> > +				if (i2c->last_msg)
> > +					return ISR_WAITSTOP;
> > +				return ISR_COMPLETE(0);
> 
> That already happens in img_i2c_isr().
> 
Will drop this patch from v2.

Sifan
> Cheers
> James

RE: [PATCH 4/7] i2c: img-scb: mark transaction as complete when no more data to write

[Sifan Naeem]

Hi James,

> On 27/07/15 12:55, Sifan Naeem wrote:
> > We can mark the transfer as complete without waiting for the stop bit.
> > This is important when handling repeated start transfers as we have to
> > start the next transfer without the stop bit being issued.
> >
> > This doesn't affect the older versions of the IP.
> >
> > Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |   15 +++++----------
> >  1 file changed, 5 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index e27c3e0..efad4d7 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -911,16 +911,11 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> >  		}
> >  	} else {
> >  		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
> > -			/*
> > -			 * The write fifo empty indicates that we're in the
> > -			 * last byte so it's safe to start a new write
> > -			 * transaction without losing any bytes from the
> > -			 * previous one.
> > -			 * see 2.3.7 Repeated Start Transactions.
> > -			 */
> > -			if ((int_status & INT_FIFO_EMPTY) &&
> > -			    i2c->msg.len == 0)
> > -				return ISR_WAITSTOP;
> > +			if (i2c->msg.len == 0) {
> > +				if (i2c->last_msg)
> > +					return ISR_WAITSTOP;
> > +				return ISR_COMPLETE(0);
> > +			}
> 
> Again, already happens in img_i2c_isr().
> 
Will drop this patch from v2.

Sifan
> Cheers
> James
> 
> >  			img_i2c_write_fifo(i2c);
> >  		}
> >  	}
> >

Re: [PATCH 6/7] i2c: img-scb: add handle for stop detected interrupt

[James Hogan]

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

On 27/07/15 12:56, Sifan Naeem wrote:
> Stop Detected interrupt is triggered when a Stop bit is detected on
> the bus, which indicates the end of the current transfer.
> 
> When the end of a transfer is indicated by the Stop bit interrupt,
> drain the FIFO and signal completion for the transaction. But if the
> interrupt was triggered before all data is written to the fifo or with
> more data expected return error with transfer complete signal.
> 
> Halting the bus is no longer necessary after a stop bit is detected
> on the bus, as there cannot be a repeated start transfer when the stop
> bit has been issued, hence remove the transaction halt bit.
> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 10141a9..90faf48 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -152,6 +152,7 @@
>  #define INT_TRANSACTION_DONE		BIT(15)
>  #define INT_SLAVE_EVENT			BIT(16)
>  #define INT_TIMING			BIT(18)
> +#define INT_STOP_DETECTED		BIT(19)
>  
>  #define INT_FIFO_FULL_FILLING	(INT_FIFO_FULL  | INT_FIFO_FILLING)
>  
> @@ -175,7 +176,8 @@
>  					 INT_WRITE_ACK_ERR    | \
>  					 INT_FIFO_FULL        | \
>  					 INT_FIFO_FILLING     | \
> -					 INT_FIFO_EMPTY)
> +					 INT_FIFO_EMPTY       | \
> +					 INT_STOP_DETECTED)
>  
>  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
>  					 INT_ADDR_ACK_ERR     | \
> @@ -907,6 +909,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  				return ISR_COMPLETE(0);
>  			}
>  		}
> +		if (int_status & INT_STOP_DETECTED) {
> +			int ret;
> +			/*
> +			 * Stop bit indicates the end of the transfer, it means
> +			 * we should read all the data (or drain the FIFO). We
> +			 * must signal completion for this transaction.
> +			 */
> +			img_i2c_transaction_halt(i2c, false);

to get a stop bit detected, wouldn't transaction halt already have to be
off?

> +			img_i2c_read_fifo(i2c);
> +			ret = (i2c->msg.len == 0) ? 0 : EIO;

If it wasn't fully transferred, wouldn't that already imply an
INT_WRITE_ACK_ERR or INT_ADDR_ACK_ERR, which should've already been
handled?

Could it then be as simple as this? (untested):

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 00ffd6613680..f694b47dcf74 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -867,6 +867,13 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 
 	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
 
+	if (int_status & INT_STOP_DETECTED) {
+		/* Drain remaining data in FIFO and complete transaction */
+		if (i2c->msg.flags & I2C_M_RD)
+			img_i2c_read_fifo(i2c);
+		return ISR_COMPLETE(0);
+	}
+
 	if (i2c->msg.flags & I2C_M_RD) {
 		if (int_status & INT_FIFO_FULL_FILLING) {
 			img_i2c_read_fifo(i2c);

Cheers
James

> +			return ISR_COMPLETE(ret);
> +		}
>  	} else {
>  		if (int_status & INT_FIFO_EMPTY) {
>  			if (i2c->msg.len == 0) {
> @@ -916,6 +930,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  			}
>  			img_i2c_write_fifo(i2c);
>  		}
> +		if (int_status & INT_STOP_DETECTED) {
> +			int ret;
> +
> +			img_i2c_transaction_halt(i2c, false);
> +			/*
> +			 * Stop bit indicates the end of a transfer and if the
> +			 * transfer has ended before all data is written to the
> +			 * fifo, return an error with transfer complete signal.
> +			 */
> +			ret = (i2c->msg.len == 0) ? 0 : EIO;
> +			return ISR_COMPLETE(ret);
> +		}
>  	}
>  
>  	return 0;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 7/7] i2c: img-scb: add handle for Master halt interrupt

[James Hogan]

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

On 27/07/15 12:56, Sifan Naeem wrote:
> Master halt is issued after each byte of a transaction is processed in
> IP version 3.3.
> Master halt will stall the bus by holding the SCK line low until the
> halt bit in the scb_general_control is cleared.
> 
> After the last byte of a transfer is processed we can use the Master
> Halt interrupt to facilitate a repeated start transfer without
> issuing a stop bit.
> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 90faf48..df3d25a 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -151,6 +151,7 @@
>  #define INT_FIFO_EMPTYING		BIT(12)
>  #define INT_TRANSACTION_DONE		BIT(15)
>  #define INT_SLAVE_EVENT			BIT(16)
> +#define INT_MASTER_HALTED		BIT(17)
>  #define INT_TIMING			BIT(18)
>  #define INT_STOP_DETECTED		BIT(19)
>  
> @@ -177,6 +178,7 @@
>  					 INT_FIFO_FULL        | \
>  					 INT_FIFO_FILLING     | \
>  					 INT_FIFO_EMPTY       | \
> +					 INT_MASTER_HALTED    | \
>  					 INT_STOP_DETECTED)
>  
>  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
> @@ -901,6 +903,17 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
>  
>  	if (i2c->msg.flags & I2C_M_RD) {
> +		if (int_status & INT_MASTER_HALTED) {
> +			img_i2c_read_fifo(i2c);
> +			if (i2c->msg.len == 0)
> +				return ISR_COMPLETE(0);

don't you still need to wait for stop bit on last message?

I suspect you could have a bit less duplication with something like this
(again untested):

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index f694b47dcf74..2de2d63083e5 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -875,13 +875,14 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 	}
 
 	if (i2c->msg.flags & I2C_M_RD) {
-		if (int_status & INT_FIFO_FULL_FILLING) {
+		if (int_status & (INT_FIFO_FULL_FILLING | INT_MASTER_HALTED)) {
 			img_i2c_read_fifo(i2c);
 			if (i2c->msg.len == 0)
 				return ISR_WAITSTOP;
 		}
 	} else {
-		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
+		if (int_status & (INT_FIFO_EMPTY_EMPTYING |
+				  INT_MASTER_HALTED)) {
 			/*
 			 * The write fifo empty indicates that we're in the
 			 * last byte so it's safe to start a new write
@@ -895,6 +896,14 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 			img_i2c_write_fifo(i2c);
 		}
 	}
+	if (int_status & INT_MASTER_HALTED) {
+		/*
+		 * Release and then enable transaction halt, to allow only a
+		 * single byte to proceed.
+		 */
+		img_i2c_transaction_halt(i2c, false);
+		img_i2c_transaction_halt(i2c, !i2c->last_msg);
+	}
 
 	return 0;
 }

would that do the trick?

Cheers
James

> +			/*
> +			 * Release and then enable transaction halt, to
> +			 * allow only a single byte to proceed.
> +			 */
> +			img_i2c_transaction_halt(i2c, false);
> +			img_i2c_transaction_halt(i2c, !i2c->last_msg);
> +		}
>  		if (int_status & INT_FIFO_FULL_FILLING) {
>  			img_i2c_read_fifo(i2c);
>  			if (i2c->msg.len == 0) {
> @@ -922,6 +935,18 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  			return ISR_COMPLETE(ret);
>  		}
>  	} else {
> +		if (int_status & INT_MASTER_HALTED) {
> +			if ((int_status & INT_FIFO_EMPTY) &&
> +			    i2c->msg.len == 0)
> +				return ISR_COMPLETE(0);
> +			img_i2c_write_fifo(i2c);
> +			/*
> +			 * Release and then enable transaction halt, to
> +			 * allow only a single byte to proceed.
> +			 */
> +			img_i2c_transaction_halt(i2c, false);
> +			img_i2c_transaction_halt(i2c, !i2c->last_msg);
> +		}
>  		if (int_status & INT_FIFO_EMPTY) {
>  			if (i2c->msg.len == 0) {
>  				if (i2c->last_msg)
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 5/7] i2c: img-scb: remove fifo EMPTYING interrupts handle

[James Hogan]

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

On 27/07/15 12:56, Sifan Naeem wrote:
> This interrupt could have been useful for repeated start transfers
> as the current transfer could be marked as complete while it's
> processing the final byte of the transfer.
> But having to use the transaction halt interrupt to safely control
> repeated start transfers, means handling of the fifo EMPTYING
> interrupts is no longer necessary.
> 
> Handling this interrupt along with Transaction Halt interrupt can
> cause erratic behaviour.

I'd be interested to know the cause of the erratic behaviour. It feels a
little like we're just masking a real bug somewhere.

Cheers
James

> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |    6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index efad4d7..10141a9 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -154,7 +154,6 @@
>  #define INT_TIMING			BIT(18)
>  
>  #define INT_FIFO_FULL_FILLING	(INT_FIFO_FULL  | INT_FIFO_FILLING)
> -#define INT_FIFO_EMPTY_EMPTYING	(INT_FIFO_EMPTY | INT_FIFO_EMPTYING)
>  
>  /* Level interrupts need clearing after handling instead of before */
>  #define INT_LEVEL			0x01e00
> @@ -176,8 +175,7 @@
>  					 INT_WRITE_ACK_ERR    | \
>  					 INT_FIFO_FULL        | \
>  					 INT_FIFO_FILLING     | \
> -					 INT_FIFO_EMPTY       | \
> -					 INT_FIFO_EMPTYING)
> +					 INT_FIFO_EMPTY)
>  
>  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
>  					 INT_ADDR_ACK_ERR     | \
> @@ -910,7 +908,7 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  			}
>  		}
>  	} else {
> -		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
> +		if (int_status & INT_FIFO_EMPTY) {
>  			if (i2c->msg.len == 0) {
>  				if (i2c->last_msg)
>  					return ISR_WAITSTOP;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH 5/7] i2c: img-scb: remove fifo EMPTYING interrupts handle

[Sifan Naeem]

Hi James,

> -----Original Message-----
> From: James Hogan
> Sent: 29 July 2015 17:01
> To: Sifan Naeem; Wolfram Sang; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 5/7] i2c: img-scb: remove fifo EMPTYING interrupts
> handle
> 
> On 27/07/15 12:56, Sifan Naeem wrote:
> > This interrupt could have been useful for repeated start transfers as
> > the current transfer could be marked as complete while it's processing
> > the final byte of the transfer.
> > But having to use the transaction halt interrupt to safely control
> > repeated start transfers, means handling of the fifo EMPTYING
> > interrupts is no longer necessary.
> >
> > Handling this interrupt along with Transaction Halt interrupt can
> > cause erratic behaviour.
> 
> I'd be interested to know the cause of the erratic behaviour. It feels a little
> like we're just masking a real bug somewhere.
> 

EMPTYING interrupt indicates that the transfer is in its last byte, and in old ip versions it was safe to start a new transfer at this point.
The erratic behaviour I saw was due to how the latest IP handles Master Halt. In this IP a transaction is halted after each byte of a transfer.
Having to halt the transfer after the last byte means we can no longer service the EMPTYING interrupt, doing so may cause repeated start transfers to fails.

Thanks,
Sifan
> Cheers
> James
> 
> >
> > Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |    6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index efad4d7..10141a9 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -154,7 +154,6 @@
> >  #define INT_TIMING			BIT(18)
> >
> >  #define INT_FIFO_FULL_FILLING	(INT_FIFO_FULL  |
> INT_FIFO_FILLING)
> > -#define INT_FIFO_EMPTY_EMPTYING	(INT_FIFO_EMPTY |
> INT_FIFO_EMPTYING)
> >
> >  /* Level interrupts need clearing after handling instead of before */
> >  #define INT_LEVEL			0x01e00
> > @@ -176,8 +175,7 @@
> >  					 INT_WRITE_ACK_ERR    | \
> >  					 INT_FIFO_FULL        | \
> >  					 INT_FIFO_FILLING     | \
> > -					 INT_FIFO_EMPTY       | \
> > -					 INT_FIFO_EMPTYING)
> > +					 INT_FIFO_EMPTY)
> >
> >  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
> >  					 INT_ADDR_ACK_ERR     | \
> > @@ -910,7 +908,7 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
> >  			}
> >  		}
> >  	} else {
> > -		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
> > +		if (int_status & INT_FIFO_EMPTY) {
> >  			if (i2c->msg.len == 0) {
> >  				if (i2c->last_msg)
> >  					return ISR_WAITSTOP;
> >

RE: [PATCH 6/7] i2c: img-scb: add handle for stop detected interrupt

[Sifan Naeem]

Hi James,

> -----Original Message-----
> From: James Hogan
> Sent: 29 July 2015 16:34
> To: Sifan Naeem; Wolfram Sang; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 6/7] i2c: img-scb: add handle for stop detected interrupt
> 
> On 27/07/15 12:56, Sifan Naeem wrote:
> > Stop Detected interrupt is triggered when a Stop bit is detected on
> > the bus, which indicates the end of the current transfer.
> >
> > When the end of a transfer is indicated by the Stop bit interrupt,
> > drain the FIFO and signal completion for the transaction. But if the
> > interrupt was triggered before all data is written to the fifo or with
> > more data expected return error with transfer complete signal.
> >
> > Halting the bus is no longer necessary after a stop bit is detected on
> > the bus, as there cannot be a repeated start transfer when the stop
> > bit has been issued, hence remove the transaction halt bit.
> >
> > Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |   28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index 10141a9..90faf48 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -152,6 +152,7 @@
> >  #define INT_TRANSACTION_DONE		BIT(15)
> >  #define INT_SLAVE_EVENT			BIT(16)
> >  #define INT_TIMING			BIT(18)
> > +#define INT_STOP_DETECTED		BIT(19)
> >
> >  #define INT_FIFO_FULL_FILLING	(INT_FIFO_FULL  |
> INT_FIFO_FILLING)
> >
> > @@ -175,7 +176,8 @@
> >  					 INT_WRITE_ACK_ERR    | \
> >  					 INT_FIFO_FULL        | \
> >  					 INT_FIFO_FILLING     | \
> > -					 INT_FIFO_EMPTY)
> > +					 INT_FIFO_EMPTY       | \
> > +					 INT_STOP_DETECTED)
> >
> >  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
> >  					 INT_ADDR_ACK_ERR     | \
> > @@ -907,6 +909,18 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> >  				return ISR_COMPLETE(0);
> >  			}
> >  		}
> > +		if (int_status & INT_STOP_DETECTED) {
> > +			int ret;
> > +			/*
> > +			 * Stop bit indicates the end of the transfer, it means
> > +			 * we should read all the data (or drain the FIFO). We
> > +			 * must signal completion for this transaction.
> > +			 */
> > +			img_i2c_transaction_halt(i2c, false);
> 
> to get a stop bit detected, wouldn't transaction halt already have to be off?

Yes, but in case we were too slow re-enabling the master halt and a stop was detected simultaneously to when master halt was set.
> 
> > +			img_i2c_read_fifo(i2c);
> > +			ret = (i2c->msg.len == 0) ? 0 : EIO;
> 
> If it wasn't fully transferred, wouldn't that already imply an
> INT_WRITE_ACK_ERR or INT_ADDR_ACK_ERR, which should've already been
> handled?
> 
This was added as a safety check, yes I guess it's impossible to get a stop bit before all data is transferred without an error condition.
>
> Could it then be as simple as this? (untested):
> 
Yes, this would do.

Thanks,
Sifan
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-
> scb.c
> index 00ffd6613680..f694b47dcf74 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -867,6 +867,13 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
> 
>  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
> 
> +	if (int_status & INT_STOP_DETECTED) {
> +		/* Drain remaining data in FIFO and complete transaction */
> +		if (i2c->msg.flags & I2C_M_RD)
> +			img_i2c_read_fifo(i2c);
> +		return ISR_COMPLETE(0);
> +	}
> +
>  	if (i2c->msg.flags & I2C_M_RD) {
>  		if (int_status & INT_FIFO_FULL_FILLING) {
>  			img_i2c_read_fifo(i2c);
> 
> Cheers
> James
> 
> > +			return ISR_COMPLETE(ret);
> > +		}
> >  	} else {
> >  		if (int_status & INT_FIFO_EMPTY) {
> >  			if (i2c->msg.len == 0) {
> > @@ -916,6 +930,18 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> >  			}
> >  			img_i2c_write_fifo(i2c);
> >  		}
> > +		if (int_status & INT_STOP_DETECTED) {
> > +			int ret;
> > +
> > +			img_i2c_transaction_halt(i2c, false);
> > +			/*
> > +			 * Stop bit indicates the end of a transfer and if the
> > +			 * transfer has ended before all data is written to the
> > +			 * fifo, return an error with transfer complete signal.
> > +			 */
> > +			ret = (i2c->msg.len == 0) ? 0 : EIO;
> > +			return ISR_COMPLETE(ret);
> > +		}
> >  	}
> >
> >  	return 0;
> >

RE: [PATCH 7/7] i2c: img-scb: add handle for Master halt interrupt

[Sifan Naeem]

Hi James,

> -----Original Message-----
> From: James Hogan
> Sent: 29 July 2015 16:59
> To: Sifan Naeem; Wolfram Sang; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 7/7] i2c: img-scb: add handle for Master halt interrupt
> 
> On 27/07/15 12:56, Sifan Naeem wrote:
> > Master halt is issued after each byte of a transaction is processed in
> > IP version 3.3.
> > Master halt will stall the bus by holding the SCK line low until the
> > halt bit in the scb_general_control is cleared.
> >
> > After the last byte of a transfer is processed we can use the Master
> > Halt interrupt to facilitate a repeated start transfer without issuing
> > a stop bit.
> >
> > Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |   25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index 90faf48..df3d25a 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -151,6 +151,7 @@
> >  #define INT_FIFO_EMPTYING		BIT(12)
> >  #define INT_TRANSACTION_DONE		BIT(15)
> >  #define INT_SLAVE_EVENT			BIT(16)
> > +#define INT_MASTER_HALTED		BIT(17)
> >  #define INT_TIMING			BIT(18)
> >  #define INT_STOP_DETECTED		BIT(19)
> >
> > @@ -177,6 +178,7 @@
> >  					 INT_FIFO_FULL        | \
> >  					 INT_FIFO_FILLING     | \
> >  					 INT_FIFO_EMPTY       | \
> > +					 INT_MASTER_HALTED    | \
> >  					 INT_STOP_DETECTED)
> >
> >  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
> > @@ -901,6 +903,17 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> >  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
> >
> >  	if (i2c->msg.flags & I2C_M_RD) {
> > +		if (int_status & INT_MASTER_HALTED) {
> > +			img_i2c_read_fifo(i2c);
> > +			if (i2c->msg.len == 0)
> > +				return ISR_COMPLETE(0);
> 
> don't you still need to wait for stop bit on last message?
> 
> I suspect you could have a bit less duplication with something like this (again
> untested):
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-
> scb.c
> index f694b47dcf74..2de2d63083e5 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -875,13 +875,14 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  	}
> 
>  	if (i2c->msg.flags & I2C_M_RD) {
> -		if (int_status & INT_FIFO_FULL_FILLING) {
> +		if (int_status & (INT_FIFO_FULL_FILLING |
> INT_MASTER_HALTED)) {
>  			img_i2c_read_fifo(i2c);
>  			if (i2c->msg.len == 0)
>  				return ISR_WAITSTOP;
>  		}
>  	} else {
> -		if (int_status & INT_FIFO_EMPTY_EMPTYING) {
> +		if (int_status & (INT_FIFO_EMPTY_EMPTYING |
> +				  INT_MASTER_HALTED)) {
>  			/*
>  			 * The write fifo empty indicates that we're in the
>  			 * last byte so it's safe to start a new write @@ -895,6
> +896,14 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  			img_i2c_write_fifo(i2c);
>  		}
>  	}
> +	if (int_status & INT_MASTER_HALTED) {
> +		/*
> +		 * Release and then enable transaction halt, to allow only a
> +		 * single byte to proceed.
> +		 */
> +		img_i2c_transaction_halt(i2c, false);
> +		img_i2c_transaction_halt(i2c, !i2c->last_msg);
> +	}
> 
>  	return 0;
>  }
> 
> would that do the trick?
> 
Yes, I'll try this. Getting rid of patches 3/7 and 4/7 makes it easier to squash these together.

Sifan

> Cheers
> James
> 
> > +			/*
> > +			 * Release and then enable transaction halt, to
> > +			 * allow only a single byte to proceed.
> > +			 */
> > +			img_i2c_transaction_halt(i2c, false);
> > +			img_i2c_transaction_halt(i2c, !i2c->last_msg);
> > +		}
> >  		if (int_status & INT_FIFO_FULL_FILLING) {
> >  			img_i2c_read_fifo(i2c);
> >  			if (i2c->msg.len == 0) {
> > @@ -922,6 +935,18 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> >  			return ISR_COMPLETE(ret);
> >  		}
> >  	} else {
> > +		if (int_status & INT_MASTER_HALTED) {
> > +			if ((int_status & INT_FIFO_EMPTY) &&
> > +			    i2c->msg.len == 0)
> > +				return ISR_COMPLETE(0);
> > +			img_i2c_write_fifo(i2c);
> > +			/*
> > +			 * Release and then enable transaction halt, to
> > +			 * allow only a single byte to proceed.
> > +			 */
> > +			img_i2c_transaction_halt(i2c, false);
> > +			img_i2c_transaction_halt(i2c, !i2c->last_msg);
> > +		}
> >  		if (int_status & INT_FIFO_EMPTY) {
> >  			if (i2c->msg.len == 0) {
> >  				if (i2c->last_msg)
> >

Re: [PATCH 6/7] i2c: img-scb: add handle for stop detected interrupt

[James Hogan]

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

On Wed, Jul 29, 2015 at 06:06:24PM +0100, Sifan Naeem wrote:
> Hi James,
> 
> > -----Original Message-----
> > From: James Hogan
> > Sent: 29 July 2015 16:34
> > To: Sifan Naeem; Wolfram Sang; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Re: [PATCH 6/7] i2c: img-scb: add handle for stop detected interrupt
> > 
> > On 27/07/15 12:56, Sifan Naeem wrote:
> > > Stop Detected interrupt is triggered when a Stop bit is detected on
> > > the bus, which indicates the end of the current transfer.
> > >
> > > When the end of a transfer is indicated by the Stop bit interrupt,
> > > drain the FIFO and signal completion for the transaction. But if the
> > > interrupt was triggered before all data is written to the fifo or with
> > > more data expected return error with transfer complete signal.
> > >
> > > Halting the bus is no longer necessary after a stop bit is detected on
> > > the bus, as there cannot be a repeated start transfer when the stop
> > > bit has been issued, hence remove the transaction halt bit.
> > >
> > > Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  drivers/i2c/busses/i2c-img-scb.c |   28 +++++++++++++++++++++++++++-
> > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > > b/drivers/i2c/busses/i2c-img-scb.c
> > > index 10141a9..90faf48 100644
> > > --- a/drivers/i2c/busses/i2c-img-scb.c
> > > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > > @@ -152,6 +152,7 @@
> > >  #define INT_TRANSACTION_DONE		BIT(15)
> > >  #define INT_SLAVE_EVENT			BIT(16)
> > >  #define INT_TIMING			BIT(18)
> > > +#define INT_STOP_DETECTED		BIT(19)
> > >
> > >  #define INT_FIFO_FULL_FILLING	(INT_FIFO_FULL  |
> > INT_FIFO_FILLING)
> > >
> > > @@ -175,7 +176,8 @@
> > >  					 INT_WRITE_ACK_ERR    | \
> > >  					 INT_FIFO_FULL        | \
> > >  					 INT_FIFO_FILLING     | \
> > > -					 INT_FIFO_EMPTY)
> > > +					 INT_FIFO_EMPTY       | \
> > > +					 INT_STOP_DETECTED)
> > >
> > >  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
> > >  					 INT_ADDR_ACK_ERR     | \
> > > @@ -907,6 +909,18 @@ static unsigned int img_i2c_auto(struct img_i2c
> > *i2c,
> > >  				return ISR_COMPLETE(0);
> > >  			}
> > >  		}
> > > +		if (int_status & INT_STOP_DETECTED) {
> > > +			int ret;
> > > +			/*
> > > +			 * Stop bit indicates the end of the transfer, it means
> > > +			 * we should read all the data (or drain the FIFO). We
> > > +			 * must signal completion for this transaction.
> > > +			 */
> > > +			img_i2c_transaction_halt(i2c, false);
> > 
> > to get a stop bit detected, wouldn't transaction halt already have to be off?
> 
> Yes, but in case we were too slow re-enabling the master halt and a stop was detected simultaneously to when master halt was set.

Okay, I already checked to make sure the t_halt thing is being done with
the spinlock held in both places, and irqs disabled in non-irq context,
since you wouldn't want a preemption or irq in between to delay t_halt
getting set anyway, in case it reached the stop bit without halting.

So you should be okay. On UP you'll never handle an irq in between as
irqs are disabled, and on SMP the spinlock will ensure the bulk of the
irq handler and t_halt thing in user context can't run concurrently on
different CPUs.

Thanks
James

> > 
> > > +			img_i2c_read_fifo(i2c);
> > > +			ret = (i2c->msg.len == 0) ? 0 : EIO;
> > 
> > If it wasn't fully transferred, wouldn't that already imply an
> > INT_WRITE_ACK_ERR or INT_ADDR_ACK_ERR, which should've already been
> > handled?
> > 
> This was added as a safety check, yes I guess it's impossible to get a stop bit before all data is transferred without an error condition.
> >
> > Could it then be as simple as this? (untested):
> > 
> Yes, this would do.
> 
> Thanks,
> Sifan
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-
> > scb.c
> > index 00ffd6613680..f694b47dcf74 100644
> > --- a/drivers/i2c/busses/i2c-img-scb.c
> > +++ b/drivers/i2c/busses/i2c-img-scb.c
> > @@ -867,6 +867,13 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
> > 
> >  	mod_timer(&i2c->check_timer, jiffies + msecs_to_jiffies(1));
> > 
> > +	if (int_status & INT_STOP_DETECTED) {
> > +		/* Drain remaining data in FIFO and complete transaction */
> > +		if (i2c->msg.flags & I2C_M_RD)
> > +			img_i2c_read_fifo(i2c);
> > +		return ISR_COMPLETE(0);
> > +	}
> > +
> >  	if (i2c->msg.flags & I2C_M_RD) {
> >  		if (int_status & INT_FIFO_FULL_FILLING) {
> >  			img_i2c_read_fifo(i2c);
> > 
> > Cheers
> > James
> > 
> > > +			return ISR_COMPLETE(ret);
> > > +		}
> > >  	} else {
> > >  		if (int_status & INT_FIFO_EMPTY) {
> > >  			if (i2c->msg.len == 0) {
> > > @@ -916,6 +930,18 @@ static unsigned int img_i2c_auto(struct img_i2c
> > *i2c,
> > >  			}
> > >  			img_i2c_write_fifo(i2c);
> > >  		}
> > > +		if (int_status & INT_STOP_DETECTED) {
> > > +			int ret;
> > > +
> > > +			img_i2c_transaction_halt(i2c, false);
> > > +			/*
> > > +			 * Stop bit indicates the end of a transfer and if the
> > > +			 * transfer has ended before all data is written to the
> > > +			 * fifo, return an error with transfer complete signal.
> > > +			 */
> > > +			ret = (i2c->msg.len == 0) ? 0 : EIO;
> > > +			return ISR_COMPLETE(ret);
> > > +		}
> > >  	}
> > >
> > >  	return 0;
> > >
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]