[PATCH v2 0/3] Cadence I2C driver fixes

[PATCH v2 0/3] 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 transfer 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 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

Vishnu Motghare (1):
  i2c: cadence: Set the hardware time-out register to maximum value

 drivers/i2c/busses/i2c-cadence.c |  178 ++++++++++++++++++++++----------------
 1 file changed, 103 insertions(+), 75 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/3] 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 - this is in sync with the new
transfer handling.

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

v2:
No changes

---
 drivers/i2c/busses/i2c-cadence.c |  156 ++++++++++++++++++++------------------
 1 file changed, 81 insertions(+), 75 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 63f3f03..e54899e 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,91 @@ 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);
-		}
+	updatetx = 0;
+	if (id->recv_count > id->curr_recv_count)
+		updatetx = 1;
+
+	/* When receiving, handle data and transfer complete interrupts */
+	if (id->p_recv_buf &&
+	    ((isr_status & CDNS_I2C_IXR_COMP) ||
+	     (isr_status & CDNS_I2C_IXR_DATA))) {
+		while (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) &
+		       CDNS_I2C_SR_RXDV) {
+			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;
-	}
+		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.
-			 */
-			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 {
+		}
+
+		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 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.
+		 */
+		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 +292,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 +317,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 +338,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

Re: [PATCH v2 1/3] i2c: cadence: Handle > 252 byte transfers

[Wolfram Sang]

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


> +		/*
> +		 * 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.
> +		 */

This comment looks broken. In general, I think there should be more
comments explaining why things have to be done this way.


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

Re: [PATCH v2 1/3] i2c: cadence: Handle > 252 byte transfers

[Harini Katakam]

Hi,

On Fri, Dec 5, 2014 at 12:02 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
>> +             /*
>> +              * 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.
>> +              */
>
> This comment looks broken. In general, I think there should be more
> comments explaining why things have to be done this way.
>

There's some grammatical errors here. Let me correct it and add more
comments.

Regards,
Harini

Re: [PATCH v2 1/3] i2c: cadence: Handle > 252 byte transfers

[rajeev kumar]

On Wed, Dec 3, 2014 at 6:05 PM, Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> 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.

Why 252 Bytes ?  Is it word allign or what ?

> The interrupt status is cleared at the beginning of the isr instead of
> the end, to avoid missing any interrupts - this is in sync with the new
> transfer handling.
>

No need to write this, actually this is the correct way of handling interrupt.

> Signed-off-by: Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
>
> v2:
> No changes
>
> ---
>  drivers/i2c/busses/i2c-cadence.c |  156 ++++++++++++++++++++------------------
>  1 file changed, 81 insertions(+), 75 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index 63f3f03..e54899e 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

Please do the alignment properly

>   * @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;

same here..

>         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;

why you are mixing tab and space..

>         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,91 @@ 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);
> -               }
> +       updatetx = 0;
> +       if (id->recv_count > id->curr_recv_count)
> +               updatetx = 1;
> +
> +       /* When receiving, handle data and transfer complete interrupts */

Breaking statement

> +       if (id->p_recv_buf &&
> +           ((isr_status & CDNS_I2C_IXR_COMP) ||
> +            (isr_status & CDNS_I2C_IXR_DATA))) {
> +               while (cdns_i2c_readreg(CDNS_I2C_SR_OFFSET) &
> +                      CDNS_I2C_SR_RXDV) {
> +                       if ((id->recv_count < CDNS_I2C_FIFO_DEPTH) &&
> +                           !id->bus_hold_flag)
> +                               cdns_i2c_clear_bus_hold(id);

make it simple. you can use extra variables also.

>
> -               /* 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;
> -       }
> +               if (updatetx &&
> +                   (id->curr_recv_count == CDNS_I2C_FIFO_DEPTH + 1)) {
> +                       /* wait while fifo is full */

Not convinced with the implementation . you are waiting in ISR.. You
can move this part in transfer function,

> +                       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.
> -                        */
> -                       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 {
> +               }
> +
> +               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 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.
> +                */
> +               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 +292,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 +317,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 +338,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

Overall I think it is required to re-write isr.

~Rajeev

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] i2c: cadence: Handle > 252 byte transfers

[Harini Katakam]

Hi,

On Fri, Dec 5, 2014 at 11:11 AM, rajeev kumar
<rajeevkumar.linux@gmail.com> wrote:
> On Wed, Dec 3, 2014 at 6:05 PM, Harini Katakam <harinik@xilinx.com> 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.
>
> Why 252 Bytes ?  Is it word allign or what ?
>

It is the maximum transfer size that can be written that is a multiple of
the data interrupt (this occurs when the fifo has 14 bytes).
I will include an explanation in driver as well.

Regards,
Harini

Re: [PATCH v2 1/3] i2c: cadence: Handle > 252 byte transfers

[Michal Simek]

On 12/05/2014 06:41 AM, rajeev kumar wrote:
> On Wed, Dec 3, 2014 at 6:05 PM, Harini Katakam <harinik@xilinx.com> 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.
> 
> Why 252 Bytes ?  Is it word allign or what ?
> 
>> The interrupt status is cleared at the beginning of the isr instead of
>> the end, to avoid missing any interrupts - this is in sync with the new
>> transfer handling.
>>
> 
> No need to write this, actually this is the correct way of handling interrupt.
> 
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>> ---
>>
>> v2:
>> No changes
>>
>> ---
>>  drivers/i2c/busses/i2c-cadence.c |  156 ++++++++++++++++++++------------------
>>  1 file changed, 81 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
>> index 63f3f03..e54899e 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
> 
> Please do the alignment properly

Alignments are correct when you apply this patch.
Please let us know if you see any problem.

Thanks,
Michal

Re: [PATCH v2 1/3] i2c: cadence: Handle > 252 byte transfers

[Martin Hisch]

Harini Katakam <harinik@...> writes:

> 
> 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 - this is in sync with the new
> transfer handling.
> 
> Signed-off-by: Harini Katakam <harinik@...>
> ---
> 
> v2:
> No changes
> 


Hi,
just had a look to the ISR code and found:

	/* wait while fifo is full */
	while (cdns_i2c_readreg(CDNS_I2C_XFER_SIZE_OFFSET) 
	    (id->curr_recv_count - CDNS_I2C_FIFO_DEPTH))

this means, that (16 x 9 bit / SCL freq) is being waited in the ISR.
At 400 kHz SCL freq, this is 360 us. Is that a valid 
workaround in respect to realtime behaviour?

[PATCH v2 2/3] i2c: cadence: Set the hardware time-out register to maximum value

[Harini Katakam]

From: Vishnu Motghare <vishnum@xilinx.com>

Cadence I2C controller has bug wherein it generates invalid read transactions
after timeout in master receiver mode. This driver does not use the HW
timeout and this interrupt is disabled but the feature itself cannot be
disabled. Hence, this patch writes the maximum value (0xFF) to this register.
This is one of the workarounds to this bug and it will not avoid the issue
completely but reduces the chances of error.

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

v2:
Added comments in driver.

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

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index e54899e..67077c2 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -111,6 +111,8 @@
 #define CDNS_I2C_DIVA_MAX	4
 #define CDNS_I2C_DIVB_MAX	64
 
+#define CDNS_I2C_TIMEOUT_MAX	0xFF
+
 #define cdns_i2c_readreg(offset)       readl_relaxed(id->membase + offset)
 #define cdns_i2c_writereg(val, offset) writel_relaxed(val, id->membase + offset)
 
@@ -858,6 +860,15 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 		goto err_clk_dis;
 	}
 
+	/*
+	 * Cadence I2C controller has a bug wherein it generates
+	 * invalid read transaction after HW timeout in master receiver mode.
+	 * HW timeout is not used by this driver and the interrupt is disabled.
+	 * But the feature itself cannot be disabled. Hence maximum value
+	 * is written to this register to reduce the chances of error.
+	 */
+	cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET);
+
 	dev_info(&pdev->dev, "%u kHz mmio %08lx irq %d\n",
 		 id->i2c_clk / 1000, (unsigned long)r_mem->start, id->irq);
 
-- 
1.7.9.5

Re: [PATCH v2 2/3] i2c: cadence: Set the hardware time-out register to maximum value

[Wolfram Sang]

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

On Wed, Dec 03, 2014 at 06:05:25PM +0530, Harini Katakam wrote:
> From: Vishnu Motghare <vishnum@xilinx.com>
> 
> Cadence I2C controller has bug wherein it generates invalid read transactions
> after timeout in master receiver mode. This driver does not use the HW
> timeout and this interrupt is disabled but the feature itself cannot be
> disabled. Hence, this patch writes the maximum value (0xFF) to this register.
> This is one of the workarounds to this bug and it will not avoid the issue
> completely but reduces the chances of error.
> 
> Signed-off-by: Vishnu Motghare <vishnum@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

Applied to for-current, thanks!


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

[PATCH v2 3/3] 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>
---

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

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

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 67077c2..f5437e2 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -522,6 +522,17 @@ 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)
+				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 v2 3/3] i2c: cadence: Check for errata condition involving master receive

[Wolfram Sang]

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


> +		/*
> +		 * 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)
> +				return -EOPNOTSUPP;
> +		}

Yeah, a lot better. Probably it would be good to inform the user with a
warning what went wrong?


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

Re: [PATCH v2 3/3] i2c: cadence: Check for errata condition involving master receive

[Harini Katakam]

Hi,

On Fri, Dec 5, 2014 at 12:04 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> +             /*
>> +              * 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)
>> +                             return -EOPNOTSUPP;
>> +             }
>
> Yeah, a lot better. Probably it would be good to inform the user with a
> warning what went wrong?
>

Sure. I'll add that.

Regards,
Harini