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

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

[Sifan Naeem]

The 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
in place.

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

Changes from v1:
	removed v1 3/7: "mark transaction as complete when all data is read"
	removed v1 4/7: "mark transaction as complete when no more data to write"
	1/5: Acked-by added.
	2/5 reworked with J Hogan suggestions.
	3/5 reworked with J Hogan suggestions.
	4/5 reworked with J Hogan suggestions.
	5/5 moved v1 2/7: "support repeated starts on IP v3.3 to" the end

Sifan Naeem (5):
  i2c: img-scb: support I2C_M_IGNORE_NAK
  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
  i2c: img-scb: support repeated starts on IP v3.3

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

-- 
1.7.9.5

[PATCH v2 1/5] 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>
Acked-by: James Hogan <james.hogan-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
Reviewed-by: James Hartley <james.hartley-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 c16caaccc363..ad1d1df943db 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -750,7 +750,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) {
@@ -1025,20 +1027,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 v2 2/5] i2c: img-scb: remove fifo EMPTYING interrupts handle

[Sifan Naeem]

Now that we are using the transaction halt interrupt to safely control
repeated start transfers, we no longer need to handle the fifo
emptying interrupts. 

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 |   16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index ad1d1df943db..75a44e794d75 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     | \
@@ -882,16 +880,8 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 				return ISR_WAITSTOP;
 		}
 	} 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)
+		if (int_status & INT_FIFO_EMPTY) {
+			if (i2c->msg.len == 0)
 				return ISR_WAITSTOP;
 			img_i2c_write_fifo(i2c);
 		}
-- 
1.7.9.5

[PATCH v2 3/5] 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 Detected 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 |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 75a44e794d75..17e13ff475bb 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     | \
@@ -873,6 +875,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);
-- 
1.7.9.5

[PATCH v2 4/5] 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>
Reviewed-by: James Hartley <james.hartley-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 17e13ff475bb..837a73a43a6d 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      | \
@@ -883,17 +885,26 @@ 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) {
-			if (i2c->msg.len == 0)
+		if (int_status & (INT_FIFO_EMPTY | INT_MASTER_HALTED)) {
+			if ((int_status & INT_FIFO_EMPTY) &&
+			    i2c->msg.len == 0)
 				return ISR_WAITSTOP;
 			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;
-- 
1.7.9.5

[PATCH v2 5/5] 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>
Reviewed-by: James Hartley <james.hartley-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-img-scb.c |   45 ++++++++++++++++++++++++++++++--------
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
index 837a73a43a6d..7a51f39528d7 100644
--- a/drivers/i2c/busses/i2c-img-scb.c
+++ b/drivers/i2c/busses/i2c-img-scb.c
@@ -513,7 +513,17 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
 		       SCB_CONTROL_CLK_ENABLE | SCB_CONTROL_SOFT_RESET);
 }
 
-/* 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;
@@ -582,7 +592,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));
 }
 
@@ -596,7 +605,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);
 
@@ -863,7 +871,7 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
 	/* Enable transaction halt on start bit */
 	if (line_status & LINESTAT_START_BIT_DET) {
 		if (!i2c->last_msg) {
-			img_i2c_transaction_halt(i2c, true);
+			img_i2c_transaction_halt(i2c, !i2c->last_msg);
 			/* we're no longer interested in the slave event */
 			i2c->int_enable &= ~INT_SLAVE_EVENT;
 		}
@@ -1093,12 +1101,31 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
 		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
 
-		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

Re: [PATCH v2 3/5] i2c: img-scb: add handle for stop detected interrupt

[James Hogan]

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

On 14/08/15 16:50, 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 Detected 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.

This patch looks okay, but does this paragraph still apply?

Cheers
James

> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 75a44e794d75..17e13ff475bb 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     | \
> @@ -873,6 +875,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);
> 


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

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

[James Hogan]

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

On 14/08/15 16:50, 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.
> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> Reviewed-by: James Hartley <james.hartley-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   45 ++++++++++++++++++++++++++++++--------
>  1 file changed, 36 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index 837a73a43a6d..7a51f39528d7 100644
> --- a/drivers/i2c/busses/i2c-img-scb.c
> +++ b/drivers/i2c/busses/i2c-img-scb.c
> @@ -513,7 +513,17 @@ static void img_i2c_soft_reset(struct img_i2c *i2c)
>  		       SCB_CONTROL_CLK_ENABLE | SCB_CONTROL_SOFT_RESET);
>  }
>  
> -/* 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;
> @@ -582,7 +592,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));
>  }
>  
> @@ -596,7 +605,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);
>  
> @@ -863,7 +871,7 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  	/* Enable transaction halt on start bit */
>  	if (line_status & LINESTAT_START_BIT_DET) {
>  		if (!i2c->last_msg) {
> -			img_i2c_transaction_halt(i2c, true);
> +			img_i2c_transaction_halt(i2c, !i2c->last_msg);

This hunk seems pointless given the "if" conditional it resides in.

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

Cheers
James

>  			/* we're no longer interested in the slave event */
>  			i2c->int_enable &= ~INT_SLAVE_EVENT;
>  		}
> @@ -1093,12 +1101,31 @@ static int img_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  		img_i2c_writel(i2c, SCB_INT_CLEAR_REG, ~0);
>  		img_i2c_writel(i2c, SCB_CLEAR_REG, ~0);
>  
> -		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,
> 


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

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

[James Hogan]

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

On 14/08/15 16:50, Sifan Naeem wrote:
> Now that we are using the transaction halt interrupt to safely control

Is that referring to patch 4, which comes after this one?

> repeated start transfers, we no longer need to handle the fifo
> emptying interrupts. 
> 
> Handling this interrupt along with Transaction Halt interrupt can
> cause erratic behaviour.

You said in response to my question before:
> 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.

That doesn't look like its what the code did though. If emptying and not
empty, it only wrote to fifo, but didn't start the next transaction.

> 
> Signed-off-by: Sifan Naeem <sifan.naeem-1AXoQHu6uovQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-img-scb.c |   16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-img-scb.c b/drivers/i2c/busses/i2c-img-scb.c
> index ad1d1df943db..75a44e794d75 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)

img_i2c_write_fifo also clears INT_FIFO_EMPTYING from int_enable if
nothing left to write. That would seem redundant after this change.

Cheers
James

>  
>  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
>  					 INT_ADDR_ACK_ERR     | \
> @@ -882,16 +880,8 @@ static unsigned int img_i2c_auto(struct img_i2c *i2c,
>  				return ISR_WAITSTOP;
>  		}
>  	} 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)
> +		if (int_status & INT_FIFO_EMPTY) {
> +			if (i2c->msg.len == 0)
>  				return ISR_WAITSTOP;
>  			img_i2c_write_fifo(i2c);
>  		}
> 


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

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

[Sifan Naeem]

Hi James,


> -----Original Message-----
> From: James Hogan
> Sent: 03 September 2015 09:54
> To: Sifan Naeem; Wolfram Sang; linux-i2c@vger.kernel.org; Ezequiel Garcia
> Cc: Ionela Voinescu
> Subject: Re: [PATCH v2 2/5] i2c: img-scb: remove fifo EMPTYING interrupts
> handle
> 
> On 14/08/15 16:50, Sifan Naeem wrote:
> > Now that we are using the transaction halt interrupt to safely control
> 
> Is that referring to patch 4, which comes after this one?
Yes, sorry will update the commit message.

> 
> > repeated start transfers, we no longer need to handle the fifo
> > emptying interrupts.
> >
> > Handling this interrupt along with Transaction Halt interrupt can
> > cause erratic behaviour.
> 
> You said in response to my question before:
> > 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.
> 
> That doesn't look like its what the code did though. If emptying and not
> empty, it only wrote to fifo, but didn't start the next transaction.
> 
The issue might be caused by the data being written to the fifo, hence i2c->msg.len = 0, but the transfer is actually still halted. So it will need the T_halt being lifted.
And as I saw this issue intermittently, it might be a case of the order interrupts are serviced.
Where as in the old IP, after data is written it was safe to return ISR_WAITSTOP.

Thanks,
Sifan
> >
> > Signed-off-by: Sifan Naeem <sifan.naeem@imgtec.com>
> > ---
> >  drivers/i2c/busses/i2c-img-scb.c |   16 +++-------------
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-img-scb.c
> > b/drivers/i2c/busses/i2c-img-scb.c
> > index ad1d1df943db..75a44e794d75 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)
> 
> img_i2c_write_fifo also clears INT_FIFO_EMPTYING from int_enable if
> nothing left to write. That would seem redundant after this change.
> 
> Cheers
> James
> 
> >
> >  #define INT_ENABLE_MASK_WAITSTOP	(INT_SLAVE_EVENT      | \
> >  					 INT_ADDR_ACK_ERR     | \
> > @@ -882,16 +880,8 @@ static unsigned int img_i2c_auto(struct img_i2c
> *i2c,
> >  				return ISR_WAITSTOP;
> >  		}
> >  	} 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)
> > +		if (int_status & INT_FIFO_EMPTY) {
> > +			if (i2c->msg.len == 0)
> >  				return ISR_WAITSTOP;
> >  			img_i2c_write_fifo(i2c);
> >  		}
> >