[PATCH v5 0/2] Cadence I2C driver fixes

[PATCH v5 0/2] Cadence I2C driver fixes

[Harini Katakam]

This series implements workarounds for some bugs in Cadence I2C controller.

- The I2C controller when configured in Master Mode generates invalid read transactions.
When HOLD bit is set and HW timeout occurs, invalid read transactions are
generated on the bus. This will affect repeated start conditions and large
data transfer condtions where transfer_size_register has to be updated again.
The transfer size register rolls over erroneously under these conditions.
Note that this HW timeout cannot be disabled even if the interrupt is unused.
Errata link: http://www.xilinx.com/support/answers/61664.html

-The I2C controller when configured in Master Mode is the missing master completion interrupt.
During repeated start condition, the driver has to set the HOLD bit for
the set of transfer being done together and clear the HOLD bit just before
the last transfer. But the controller does not indicate completion when
a read/receive transfer is done if the HOLD bit is set. This affects
all repeated start operation where a read/receive transfer is not
the last transfer.
Errata link: http://www.xilinx.com/support/answers/61665.html

To address the above,
- >252 byte transfers are done such that transfer_size never becomes zero.
- timeout register value is increased (even though the driver doesn't use this).
- A check is performed to see if there is any transfer following a
read/receive transfer in the set of messages using repeated start.
An error is returned in such cases because if we proceed, completion interrupt
is never obtained and a SW timeout will occur.

Harini Katakam (2):
  i2c: cadence: Handle > 252 byte transfers
  i2c: cadence: Check for errata condition involving master receive

 drivers/i2c/busses/i2c-cadence.c |  185 +++++++++++++++++++++++---------------
 1 file changed, 112 insertions(+), 73 deletions(-)

-- 
1.7.9.5

[PATCH v5 1/2] i2c: cadence: Handle > 252 byte transfers

[Harini Katakam]

The I2C controller sends a NACK to the slave when transfer size register
reaches zero, irrespective of the hold bit. So, in order to handle transfers
greater than 252 bytes, the transfer size register has to be maintained at a
value >= 1. This patch implements the same.
The interrupt status is cleared at the beginning of the isr instead of
the end, to avoid missing any interrupts.

Signed-off-by: Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---

v5:
No changes

v4:
No changes

v3:
Add comments

v2:
No changes

---
 drivers/i2c/busses/i2c-cadence.c |  173 ++++++++++++++++++++++----------------
 1 file changed, 100 insertions(+), 73 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 63f3f03..5f5d4fa 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -126,6 +126,7 @@
  * @suspended:		Flag holding the device's PM status
  * @send_count:		Number of bytes still expected to send
  * @recv_count:		Number of bytes still expected to receive
+ * @curr_recv_count:	Number of bytes to be received in current transfer
  * @irq:		IRQ number
  * @input_clk:		Input clock to I2C controller
  * @i2c_clk:		Maximum I2C clock speed
@@ -144,6 +145,7 @@ struct cdns_i2c {
 	u8 suspended;
 	unsigned int send_count;
 	unsigned int recv_count;
+	unsigned int curr_recv_count;
 	int irq;
 	unsigned long input_clk;
 	unsigned int i2c_clk;
@@ -180,14 +182,15 @@ static void cdns_i2c_clear_bus_hold(struct cdns_i2c *id)
  */
 static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
 {
-	unsigned int isr_status, avail_bytes;
-	unsigned int bytes_to_recv, bytes_to_send;
+	unsigned int isr_status, avail_bytes, updatetx;
+	unsigned int bytes_to_send;
 	struct cdns_i2c *id = ptr;
 	/* Signal completion only after everything is updated */
 	int done_flag = 0;
 	irqreturn_t status = IRQ_NONE;
 
 	isr_status = cdns_i2c_readreg(CDNS_I2C_ISR_OFFSET);
+	cdns_i2c_writereg(isr_status, CDNS_I2C_ISR_OFFSET);
 
 	/* Handling nack and arbitration lost interrupt */
 	if (isr_status & (CDNS_I2C_IXR_NACK | CDNS_I2C_IXR_ARB_LOST)) {
@@ -195,89 +198,112 @@ static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
 		status = IRQ_HANDLED;
 	}
 
-	/* Handling Data interrupt */
-	if ((isr_status & CDNS_I2C_IXR_DATA) &&
-			(id->recv_count >= CDNS_I2C_DATA_INTR_DEPTH)) {
-		/* Always read data interrupt threshold bytes */
-		bytes_to_recv = CDNS_I2C_DATA_INTR_DEPTH;
-		id->recv_count -= CDNS_I2C_DATA_INTR_DEPTH;
-		avail_bytes = cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
-
-		/*
-		 * if the tranfer size register value is zero, then
-		 * check for the remaining bytes and update the
-		 * transfer size register.
-		 */
-		if (!avail_bytes) {
-			if (id->recv_count > CDNS_I2C_TRANSFER_SIZE)
-				cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
-						CDNS_I2C_XFER_SIZE_OFFSET);
-			else
-				cdns_i2c_writereg(id->recv_count,
-						CDNS_I2C_XFER_SIZE_OFFSET);
-		}
+	/*
+	 * Check if transfer size register needs to be updated again for a
+	 * large data receive operation.
+	 */
+	updatetx = 0;
+	if (id->recv_count > id->curr_recv_count)
+		updatetx = 1;
+
+	/* When receiving, handle data interrupt and completion interrupt */
+	if (id->p_recv_buf &&
+	    ((isr_status & CDNS_I2C_IXR_COMP) ||
+	     (isr_status & CDNS_I2C_IXR_DATA))) {
+		/* Read data if receive data valid is set */
+		while (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) &
+		       CDNS_I2C_SR_RXDV) {
+			/*
+			 * Clear hold bit that was set for FIFO control if
+			 * RX data left is less than FIFO depth, unless
+			 * repeated start is selected.
+			 */
+			if ((id->recv_count < CDNS_I2C_FIFO_DEPTH) &&
+			    !id->bus_hold_flag)
+				cdns_i2c_clear_bus_hold(id);
 
-		/* Process the data received */
-		while (bytes_to_recv--)
 			*(id->p_recv_buf)++ =
 				cdns_i2c_readreg(CDNS_I2C_DATA_OFFSET);
+			id->recv_count--;
+			id->curr_recv_count--;
 
-		if (!id->bus_hold_flag &&
-				(id->recv_count <= CDNS_I2C_FIFO_DEPTH))
-			cdns_i2c_clear_bus_hold(id);
+			if (updatetx &&
+			    (id->curr_recv_count == CDNS_I2C_FIFO_DEPTH + 1))
+				break;
+		}
 
-		status = IRQ_HANDLED;
-	}
+		/*
+		 * The controller sends NACK to the slave when transfer size
+		 * register reaches zero without considering the HOLD bit.
+		 * This workaround is implemented for large data transfers to
+		 * maintain transfer size non-zero while performing a large
+		 * receive operation.
+		 */
+		if (updatetx &&
+		    (id->curr_recv_count == CDNS_I2C_FIFO_DEPTH + 1)) {
+			/* wait while fifo is full */
+			while (cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET) !=
+			       (id->curr_recv_count - CDNS_I2C_FIFO_DEPTH))
+				;
 
-	/* Handling Transfer Complete interrupt */
-	if (isr_status & CDNS_I2C_IXR_COMP) {
-		if (!id->p_recv_buf) {
 			/*
-			 * If the device is sending data If there is further
-			 * data to be sent. Calculate the available space
-			 * in FIFO and fill the FIFO with that many bytes.
+			 * Check number of bytes to be received against maximum
+			 * transfer size and update register accordingly.
 			 */
-			if (id->send_count) {
-				avail_bytes = CDNS_I2C_FIFO_DEPTH -
-				    cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
-				if (id->send_count > avail_bytes)
-					bytes_to_send = avail_bytes;
-				else
-					bytes_to_send = id->send_count;
-
-				while (bytes_to_send--) {
-					cdns_i2c_writereg(
-						(*(id->p_send_buf)++),
-						 CDNS_I2C_DATA_OFFSET);
-					id->send_count--;
-				}
+			if (((int)(id->recv_count) - CDNS_I2C_FIFO_DEPTH) >
+			    CDNS_I2C_TRANSFER_SIZE) {
+				cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
+						  CDNS_I2C_XFER_SIZE_OFFSET);
+				id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE +
+						      CDNS_I2C_FIFO_DEPTH;
 			} else {
-				/*
-				 * Signal the completion of transaction and
-				 * clear the hold bus bit if there are no
-				 * further messages to be processed.
-				 */
-				done_flag = 1;
+				cdns_i2c_writereg(id->recv_count -
+						  CDNS_I2C_FIFO_DEPTH,
+						  CDNS_I2C_XFER_SIZE_OFFSET);
+				id->curr_recv_count = id->recv_count;
 			}
-			if (!id->send_count && !id->bus_hold_flag)
-				cdns_i2c_clear_bus_hold(id);
-		} else {
+		}
+
+		/* Clear hold (if not repeated start) and signal completion */
+		if ((isr_status & CDNS_I2C_IXR_COMP) && !id->recv_count) {
 			if (!id->bus_hold_flag)
 				cdns_i2c_clear_bus_hold(id);
+			done_flag = 1;
+		}
+
+		status = IRQ_HANDLED;
+	}
+
+	/* When sending, handle transfer complete interrupt */
+	if ((isr_status & CDNS_I2C_IXR_COMP) && !id->p_recv_buf) {
+		/*
+		 * If there is more data to be sent, calculate the
+		 * space available in FIFO and fill with that many bytes.
+		 */
+		if (id->send_count) {
+			avail_bytes = CDNS_I2C_FIFO_DEPTH -
+			    cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET);
+			if (id->send_count > avail_bytes)
+				bytes_to_send = avail_bytes;
+			else
+				bytes_to_send = id->send_count;
+
+			while (bytes_to_send--) {
+				cdns_i2c_writereg(
+					(*(id->p_send_buf)++),
+					 CDNS_I2C_DATA_OFFSET);
+				id->send_count--;
+			}
+		} else {
 			/*
-			 * If the device is receiving data, then signal
-			 * the completion of transaction and read the data
-			 * present in the FIFO. Signal the completion of
-			 * transaction.
+			 * Signal the completion of transaction and
+			 * clear the hold bus bit if there are no
+			 * further messages to be processed.
 			 */
-			while (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) &
-					CDNS_I2C_SR_RXDV) {
-				*(id->p_recv_buf)++ =
-					cdns_i2c_readreg(CDNS_I2C_DATA_OFFSET);
-				id->recv_count--;
-			}
 			done_flag = 1;
 		}
+		if (!id->send_count && !id->bus_hold_flag)
+			cdns_i2c_clear_bus_hold(id);
 
 		status = IRQ_HANDLED;
 	}
@@ -287,8 +313,6 @@ static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
 	if (id->err_status)
 		status = IRQ_HANDLED;
 
-	cdns_i2c_writereg(isr_status, CDNS_I2C_ISR_OFFSET);
-
 	if (done_flag)
 		complete(&id->xfer_done);
 
@@ -314,6 +338,8 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	if (id->p_msg->flags & I2C_M_RECV_LEN)
 		id->recv_count = I2C_SMBUS_BLOCK_MAX + 1;
 
+	id->curr_recv_count = id->recv_count;
+
 	/*
 	 * Check for the message size against FIFO depth and set the
 	 * 'hold bus' bit if it is greater than FIFO depth.
@@ -333,10 +359,11 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	 * receive if it is less than transfer size and transfer size if
 	 * it is more. Enable the interrupts.
 	 */
-	if (id->recv_count > CDNS_I2C_TRANSFER_SIZE)
+	if (id->recv_count > CDNS_I2C_TRANSFER_SIZE) {
 		cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
 				  CDNS_I2C_XFER_SIZE_OFFSET);
-	else
+		id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
+	} else
 		cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
 	/* Clear the bus hold flag if bytes to receive is less than FIFO size */
 	if (!id->bus_hold_flag &&
-- 
1.7.9.5

[PATCH v5 2/2] i2c: cadence: Check for errata condition involving master receive

[Harini Katakam]

Cadence I2C controller has the following bugs:
- completion indication is not given to the driver at the end of
a read/receive transfer with HOLD bit set.
- Invalid read transaction are generated on the bus when HW timeout
condition occurs with HOLD bit set.

As a result of the above, if a set of messages to be transferred with
repeated start includes any transfer following a read transfer,
completion is never indicated and timeout occurs.
Hence a check is implemented to return -EOPNOTSUPP for such sequences.

Signed-off-by: Harini Katakam <harinik@xilinx.com>
Signed-off-by: Vishnu Motghare <vishnum@xilinx.com>
---

v5:
Make warning grepable in driver.

v4:
Use single dev_warn and make message grep-able.

v3:
Add warning in case of unsupported transfer.

v2:
Dont defeteature repeated start. Just check for unsupported conditions in the
driver and return error.

---
 drivers/i2c/busses/i2c-cadence.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 5f5d4fa..6538c6c 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -541,6 +541,18 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	 * processed with a repeated start.
 	 */
 	if (num > 1) {
+		/*
+		 * This controller does not give completion interrupt after a
+		 * master receive transfer if HOLD bit is set (repeated start),
+		 * resulting in SW timeout. Hence, if a receive transfer is
+		 * followed by any other transfer, an error is returned
+		 * indicating that this sequence is not supported.
+		 */
+		for (count = 0; count < num-1; count++) {
+			if (msgs[count].flags & I2C_M_RD)
+				dev_warn(adap->dev.parent, "No support for repeated start when receive is followed by a transfer\n");
+				return -EOPNOTSUPP;
+		}
 		id->bus_hold_flag = 1;
 		reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
 		reg |= CDNS_I2C_CR_HOLD;
-- 
1.7.9.5

Re: [PATCH v5 0/2] Cadence I2C driver fixes

[Harini Katakam]

Hi,

Any further comments on this?

Regards,
Harini

On Fri, Dec 12, 2014 at 9:48 AM, Harini Katakam <harinik@xilinx.com> wrote:
> This series implements workarounds for some bugs in Cadence I2C controller.
>
> - The I2C controller when configured in Master Mode generates invalid read transactions.
> When HOLD bit is set and HW timeout occurs, invalid read transactions are
> generated on the bus. This will affect repeated start conditions and large
> data transfer condtions where transfer_size_register has to be updated again.
> The transfer size register rolls over erroneously under these conditions.
> Note that this HW timeout cannot be disabled even if the interrupt is unused.
> Errata link: http://www.xilinx.com/support/answers/61664.html
>
> -The I2C controller when configured in Master Mode is the missing master completion interrupt.
> During repeated start condition, the driver has to set the HOLD bit for
> the set of transfer being done together and clear the HOLD bit just before
> the last transfer. But the controller does not indicate completion when
> a read/receive transfer is done if the HOLD bit is set. This affects
> all repeated start operation where a read/receive transfer is not
> the last transfer.
> Errata link: http://www.xilinx.com/support/answers/61665.html
>
> To address the above,
> - >252 byte transfers are done such that transfer_size never becomes zero.
> - timeout register value is increased (even though the driver doesn't use this).
> - A check is performed to see if there is any transfer following a
> read/receive transfer in the set of messages using repeated start.
> An error is returned in such cases because if we proceed, completion interrupt
> is never obtained and a SW timeout will occur.
>
> Harini Katakam (2):
>   i2c: cadence: Handle > 252 byte transfers
>   i2c: cadence: Check for errata condition involving master receive
>
>  drivers/i2c/busses/i2c-cadence.c |  185 +++++++++++++++++++++++---------------
>  1 file changed, 112 insertions(+), 73 deletions(-)
>
> --
> 1.7.9.5
>

Re: [PATCH v5 1/2] i2c: cadence: Handle > 252 byte transfers

[Wolfram Sang]

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

On Fri, Dec 12, 2014 at 09:48:26AM +0530, Harini Katakam wrote:
> The I2C controller sends a NACK to the slave when transfer size register
> reaches zero, irrespective of the hold bit. So, in order to handle transfers
> greater than 252 bytes, the transfer size register has to be maintained at a
> value >= 1. This patch implements the same.
> The interrupt status is cleared at the beginning of the isr instead of
> the end, to avoid missing any interrupts.
> 
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

Applied to for-next, thanks!

> @@ -333,10 +359,11 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>  	 * receive if it is less than transfer size and transfer size if
>  	 * it is more. Enable the interrupts.
>  	 */
> -	if (id->recv_count > CDNS_I2C_TRANSFER_SIZE)
> +	if (id->recv_count > CDNS_I2C_TRANSFER_SIZE) {
>  		cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
>  				  CDNS_I2C_XFER_SIZE_OFFSET);
> -	else
> +		id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
> +	} else
>  		cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);

else branch must have braces, too! I fixed that.


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

Re: [PATCH v5 2/2] i2c: cadence: Check for errata condition involving master receive

[Wolfram Sang]

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

> @@ -541,6 +541,18 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>  	 * processed with a repeated start.
>  	 */
>  	if (num > 1) {
> +		/*
> +		 * This controller does not give completion interrupt after a
> +		 * master receive transfer if HOLD bit is set (repeated start),
> +		 * resulting in SW timeout. Hence, if a receive transfer is
> +		 * followed by any other transfer, an error is returned
> +		 * indicating that this sequence is not supported.
> +		 */
> +		for (count = 0; count < num-1; count++) {
> +			if (msgs[count].flags & I2C_M_RD)
> +				dev_warn(adap->dev.parent, "No support for repeated start when receive is followed by a transfer\n");
> +				return -EOPNOTSUPP;
> +		}
>  		id->bus_hold_flag = 1;
>  		reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
>  		reg |= CDNS_I2C_CR_HOLD;

Have you ever tested this code? There is a huge bug in it :(

Also, in the comment, use "message" instead of "transfer". One transfer
consists of multiple messages.

Maybe the warning can be simplified, too: "can't do repeated start after
read messages".


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

Re: [PATCH v5 2/2] i2c: cadence: Check for errata condition involving master receive

[Harini Katakam]

Hi,

On Tue, Jan 13, 2015 at 5:05 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> @@ -541,6 +541,18 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
>>        * processed with a repeated start.
>>        */
>>       if (num > 1) {
>> +             /*
>> +              * This controller does not give completion interrupt after a
>> +              * master receive transfer if HOLD bit is set (repeated start),
>> +              * resulting in SW timeout. Hence, if a receive transfer is
>> +              * followed by any other transfer, an error is returned
>> +              * indicating that this sequence is not supported.
>> +              */
>> +             for (count = 0; count < num-1; count++) {
>> +                     if (msgs[count].flags & I2C_M_RD)
>> +                             dev_warn(adap->dev.parent, "No support for repeated start when receive is followed by a transfer\n");
>> +                             return -EOPNOTSUPP;
>> +             }
>>               id->bus_hold_flag = 1;
>>               reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
>>               reg |= CDNS_I2C_CR_HOLD;
>
> Have you ever tested this code? There is a huge bug in it :(

I'm sorry. I'll send another patch with the braces for the if statement.
I seem to have tested with only the return statement and no dev_warn.

>
> Also, in the comment, use "message" instead of "transfer". One transfer
> consists of multiple messages.
>
> Maybe the warning can be simplified, too: "can't do repeated start after
> read messages".
>
I'll do that.

Regards,
Harini

Re: [PATCH v5 1/2] i2c: cadence: Handle > 252 byte transfers

[Harini Katakam]

Hi,

On Tue, Jan 13, 2015 at 4:58 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Fri, Dec 12, 2014 at 09:48:26AM +0530, Harini Katakam wrote:
>> The I2C controller sends a NACK to the slave when transfer size register
>> reaches zero, irrespective of the hold bit. So, in order to handle transfers
>> greater than 252 bytes, the transfer size register has to be maintained at a
>> value >= 1. This patch implements the same.
>> The interrupt status is cleared at the beginning of the isr instead of
>> the end, to avoid missing any interrupts.
>>
>> Signed-off-by: Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>
> Applied to for-next, thanks!
>
>> @@ -333,10 +359,11 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
>>        * receive if it is less than transfer size and transfer size if
>>        * it is more. Enable the interrupts.
>>        */
>> -     if (id->recv_count > CDNS_I2C_TRANSFER_SIZE)
>> +     if (id->recv_count > CDNS_I2C_TRANSFER_SIZE) {
>>               cdns_i2c_writereg(CDNS_I2C_TRANSFER_SIZE,
>>                                 CDNS_I2C_XFER_SIZE_OFFSET);
>> -     else
>> +             id->curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
>> +     } else
>>               cdns_i2c_writereg(id->recv_count, CDNS_I2C_XFER_SIZE_OFFSET);
>
> else branch must have braces, too! I fixed that.

Thanks!

Regards,
Harini