[PATCH 0/4] Cadence I2C driver fixes

[PATCH 0/4] 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).
- repeated start is defeatured based on a devicetree input. This input is
because repeated start is required as part of protocol for some slave devices
(not just to retain control in a multi-master scenario). The driver works
as expected with these devices and hence the defeature is optional in such
cases.
- In case user chooses not to de-feature repeated start, then 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 (1):
  i2c: cadence: Handle > 252 byte transfers

Vishnu Motghare (3):
  i2c: cadence: Set the hardware time-out register to maximum value
  devicetree: bindings: Add defeature-repeated-start property for
    Cadence I2C
  i2c: cadence: Defeature repeated start based on devicetree property

 .../devicetree/bindings/i2c/i2c-cadence.txt        |   11 ++
 drivers/i2c/busses/i2c-cadence.c                   |  200 +++++++++++---------
 2 files changed, 125 insertions(+), 86 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] 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@xilinx.com>
---
 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

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

[Harini Katakam]

From: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

Cadence I2C controller has bug wherein it generates invalid read transactions
after time-out 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 reduce the chances of error.

Signed-off-by: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-cadence.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index e54899e..8065205 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,8 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 		goto err_clk_dis;
 	}
 
+	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 2/4] i2c: cadence: Set the hardware time-out register to maximum value

[Wolfram Sang]

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

On Tue, Dec 02, 2014 at 03:35:47PM +0530, Harini Katakam wrote:
> From: Vishnu Motghare <vishnum@xilinx.com>
> 
> Cadence I2C controller has bug wherein it generates invalid read transactions
> after time-out 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 reduce the chances of error.

This is a good description and should be in a comment in the driver.

> 
> Signed-off-by: Vishnu Motghare <vishnum@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
>  drivers/i2c/busses/i2c-cadence.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
> index e54899e..8065205 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,8 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>  		goto err_clk_dis;
>  	}
>  
> +	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
> 

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

[PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Harini Katakam]

From: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

This patch adds "defeature-repeated-start" property in i2c-cadence.txt.

Signed-off-by: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/i2c/i2c-cadence.txt        |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
index 7cb0b56..9d417a7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
@@ -11,6 +11,17 @@ Required properties:
 Optional properties:
   - clock-frequency: Desired operating frequency, in Hz, of the bus.
   - clock-names: Input clock name, should be 'pclk'.
+  - defeature-repeated-start: Include this property to defeature repeated start
+			      This defeature is due to a few bugs in the
+			      I2C controller.
+			      Completion interrupt after a read/receive
+			      operation is NOT obtained if HOLD bit is set
+			      at that time. Because of this bug, repeated start
+			      will only work if there are no transfers following
+			      a read/receive transfer.
+			      If HOLD is held for long without a transfer,
+			      invalid read transactions are generated by the
+			      controller due to a HW timeout related bug.
 
 Example:
 	i2c@e0004000 {
-- 
1.7.9.5

Re: [PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Mark Rutland]

On Tue, Dec 02, 2014 at 10:05:48AM +0000, Harini Katakam wrote:
> From: Vishnu Motghare <vishnum@xilinx.com>
> 
> This patch adds "defeature-repeated-start" property in i2c-cadence.txt.
> 
> Signed-off-by: Vishnu Motghare <vishnum@xilinx.com>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
> ---
>  .../devicetree/bindings/i2c/i2c-cadence.txt        |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
> index 7cb0b56..9d417a7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
> @@ -11,6 +11,17 @@ Required properties:
>  Optional properties:
>    - clock-frequency: Desired operating frequency, in Hz, of the bus.
>    - clock-names: Input clock name, should be 'pclk'.
> +  - defeature-repeated-start: Include this property to defeature repeated start
> +			      This defeature is due to a few bugs in the
> +			      I2C controller.
> +			      Completion interrupt after a read/receive
> +			      operation is NOT obtained if HOLD bit is set
> +			      at that time. Because of this bug, repeated start
> +			      will only work if there are no transfers following
> +			      a read/receive transfer.
> +			      If HOLD is held for long without a transfer,
> +			      invalid read transactions are generated by the
> +			      controller due to a HW timeout related bug.

I'm not keen on the name; it sounds like we're disabling a feature
rather than describing the problem (and "defeature" is not a common
term in this sense, "disable" would be better).

It sounds like there are two issues with staying in the HOLD state? Lost
completion IRQs and a separate HW timeout bug? Or are the two related?

Thanks,
Mark.

Re: [PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Harini Katakam]

Hi Mark,

On Tue, Dec 2, 2014 at 4:49 PM, Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> wrote:
> On Tue, Dec 02, 2014 at 10:05:48AM +0000, Harini Katakam wrote:
>> From: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>>
>> This patch adds "defeature-repeated-start" property in i2c-cadence.txt.
>>
>> Signed-off-by: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>> Signed-off-by: Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
>> ---
>>  .../devicetree/bindings/i2c/i2c-cadence.txt        |   11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
>> index 7cb0b56..9d417a7 100644
>> --- a/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
>> +++ b/Documentation/devicetree/bindings/i2c/i2c-cadence.txt
>> @@ -11,6 +11,17 @@ Required properties:
>>  Optional properties:
>>    - clock-frequency: Desired operating frequency, in Hz, of the bus.
>>    - clock-names: Input clock name, should be 'pclk'.
>> +  - defeature-repeated-start: Include this property to defeature repeated start
>> +                           This defeature is due to a few bugs in the
>> +                           I2C controller.
>> +                           Completion interrupt after a read/receive
>> +                           operation is NOT obtained if HOLD bit is set
>> +                           at that time. Because of this bug, repeated start
>> +                           will only work if there are no transfers following
>> +                           a read/receive transfer.
>> +                           If HOLD is held for long without a transfer,
>> +                           invalid read transactions are generated by the
>> +                           controller due to a HW timeout related bug.
>
> I'm not keen on the name; it sounds like we're disabling a feature
> rather than describing the problem (and "defeature" is not a common
> term in this sense, "disable" would be better).
>
> It sounds like there are two issues with staying in the HOLD state? Lost
> completion IRQs and a separate HW timeout bug? Or are the two related?
>

Yes, there are two issues here and they are not related.
But a combination of both is leading to not using repeated start.
The intention was to defeature except that it works in some scenarios
(such as a typical write+read in that order with repeated start)
and there are people who already use the driver with slaves that need this.

Regards,
Harini
--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Wolfram Sang]

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

> >> +  - defeature-repeated-start: Include this property to defeature repeated start
> >> +                           This defeature is due to a few bugs in the
> >> +                           I2C controller.
> >> +                           Completion interrupt after a read/receive
> >> +                           operation is NOT obtained if HOLD bit is set
> >> +                           at that time. Because of this bug, repeated start
> >> +                           will only work if there are no transfers following
> >> +                           a read/receive transfer.
> >> +                           If HOLD is held for long without a transfer,
> >> +                           invalid read transactions are generated by the
> >> +                           controller due to a HW timeout related bug.
> >
> > I'm not keen on the name; it sounds like we're disabling a feature
> > rather than describing the problem (and "defeature" is not a common
> > term in this sense, "disable" would be better).
> >
> > It sounds like there are two issues with staying in the HOLD state? Lost
> > completion IRQs and a separate HW timeout bug? Or are the two related?
> >
> 
> Yes, there are two issues here and they are not related.
> But a combination of both is leading to not using repeated start.
> The intention was to defeature except that it works in some scenarios
> (such as a typical write+read in that order with repeated start)
> and there are people who already use the driver with slaves that need this.

That should not be handled using a binding. If you get a transfer (at
runtime) with criteria you don't support, return with -EOPNOTSUPP from
the master xfer routine.

That being said, the number of broken/not-fully-compliant I2C
controllers has increased a lot recent times (why can't we just use the
established old ones?). Maybe we will have core support for a subset of
I2C (wr+rd) in the future, but that's still ahead...


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

Re: [PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Harini Katakam]

Hi,

On Tue, Dec 2, 2014 at 6:22 PM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> >> +  - defeature-repeated-start: Include this property to defeature repeated start
>> >> +                           This defeature is due to a few bugs in the
>> >> +                           I2C controller.
>> >> +                           Completion interrupt after a read/receive
>> >> +                           operation is NOT obtained if HOLD bit is set
>> >> +                           at that time. Because of this bug, repeated start
>> >> +                           will only work if there are no transfers following
>> >> +                           a read/receive transfer.
>> >> +                           If HOLD is held for long without a transfer,
>> >> +                           invalid read transactions are generated by the
>> >> +                           controller due to a HW timeout related bug.
>> >
>> > I'm not keen on the name; it sounds like we're disabling a feature
>> > rather than describing the problem (and "defeature" is not a common
>> > term in this sense, "disable" would be better).
>> >
>> > It sounds like there are two issues with staying in the HOLD state? Lost
>> > completion IRQs and a separate HW timeout bug? Or are the two related?
>> >
>>
>> Yes, there are two issues here and they are not related.
>> But a combination of both is leading to not using repeated start.
>> The intention was to defeature except that it works in some scenarios
>> (such as a typical write+read in that order with repeated start)
>> and there are people who already use the driver with slaves that need this.
>
> That should not be handled using a binding. If you get a transfer (at
> runtime) with criteria you don't support, return with -EOPNOTSUPP from
> the master xfer routine.
>

I put a check in place for one failure condition that we know (will
change the error code returned).
But given the bugs, it will be useful to just disable it if the system doesn't
require repeated start.
If you think DT entry is not the way to go, do you think a CONFIG option or
something better will work?
We chose a DT property because there is a good chance the user has multiple
cadence I2C controllers - one connected to a slave that needs repeated start
(like a power controller) and another that doesn't care.

Regards,
Harini

Re: [PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Wolfram Sang]

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


> But given the bugs, it will be useful to just disable it if the system doesn't
> require repeated start.

What do you do when disable repeated start? Sending STOP and START? If
so, this is really something different than repeated start. By using
I2C_FUNC_I2C a user expects repeated start, so if the HW does not
support it, we should say so and don't try to emulate it with something
different.

> If you think DT entry is not the way to go, do you think a CONFIG option or
> something better will work?

No, check at runtime if the transfer is possible on this HW. Bail out if
not.

> We chose a DT property because there is a good chance the user has multiple
> cadence I2C controllers - one connected to a slave that needs repeated start
> (like a power controller) and another that doesn't care.

The user should not need to know with this level of detail if we can
avoid it IMO.


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

Re: [PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Harini Katakam]

Hi,

On Tue, Dec 2, 2014 at 6:46 PM, Wolfram Sang <wsa@the-dreams.de> wrote:
>
>> But given the bugs, it will be useful to just disable it if the system doesn't
>> require repeated start.
>
> What do you do when disable repeated start? Sending STOP and START? If
> so, this is really something different than repeated start. By using
> I2C_FUNC_I2C a user expects repeated start, so if the HW does not
> support it, we should say so and don't try to emulate it with something
> different.
>

Yes, we send stop.
Using repeated start, when number of messages passed > 1, HOLD bit is set
by the driver. This is an indication to the controller not to send a STOP.
If we disable repeated start, the driver will not set HOLD bit.
All the messages are sent but with START and a STOP for each of them.

Regards,
Harini

Re: [PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Wolfram Sang]

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

> > What do you do when disable repeated start? Sending STOP and START? If
> > so, this is really something different than repeated start. By using
> > I2C_FUNC_I2C a user expects repeated start, so if the HW does not
> > support it, we should say so and don't try to emulate it with something
> > different.
> 
> Yes, we send stop.

As said before, this is wrong. Another master could interfere between
the messages when using stop+start. This is no replacement for repeated
start.


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

Re: [PATCH 3/4] devicetree: bindings: Add defeature-repeated-start property for Cadence I2C

[Lars-Peter Clausen]

On 12/02/2014 03:15 PM, Wolfram Sang wrote:
>>> What do you do when disable repeated start? Sending STOP and START? If
>>> so, this is really something different than repeated start. By using
>>> I2C_FUNC_I2C a user expects repeated start, so if the HW does not
>>> support it, we should say so and don't try to emulate it with something
>>> different.
>>
>> Yes, we send stop.
>
> As said before, this is wrong. Another master could interfere between
> the messages when using stop+start. This is no replacement for repeated
> start.

More importantly a lot of I2C slaves also reset their internal state machine 
on a stop. So e.g. if reading a register is implemented by doing 
start,write,repeated start,read,stop and you replace that with 
start,write,stop,start,read,stop you'll always read register zero instead of 
the register you wanted to read.

- Lars

[PATCH 4/4] i2c: cadence: Defeature repeated start based on devicetree property

[Harini Katakam]

From: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>

This patch defeatures repeated start in the driver if
"defeature-repeated-start" property is present in the devicetree.

This defeature is proposed due to a few bugs in the controller
- 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.

Signed-off-by: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-cadence.c |   44 +++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 13 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 8065205..89335f7 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -135,6 +135,7 @@
  * @bus_hold_flag:	Flag used in repeated start for clearing HOLD bit
  * @clk:		Pointer to struct clk
  * @clk_rate_change_nb:	Notifier block for clock rate changes
+ * @defeature_repeated_start:	Flag to de feature repeated start
  */
 struct cdns_i2c {
 	void __iomem *membase;
@@ -154,6 +155,7 @@ struct cdns_i2c {
 	unsigned int bus_hold_flag;
 	struct clk *clk;
 	struct notifier_block clk_rate_change_nb;
+	unsigned int defeature_repeated_start;
 };
 
 #define to_cdns_i2c(_nb)	container_of(_nb, struct cdns_i2c, \
@@ -246,7 +248,7 @@ static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
 		}
 
 		if ((isr_status & CDNS_I2C_IXR_COMP) && !id->recv_count) {
-			if (!id->bus_hold_flag)
+			if (id->defeature_repeated_start || !id->bus_hold_flag)
 				cdns_i2c_clear_bus_hold(id);
 			done_flag = 1;
 		}
@@ -283,7 +285,8 @@ static irqreturn_t cdns_i2c_isr(int irq, void *ptr)
 			 */
 			done_flag = 1;
 		}
-		if (!id->send_count && !id->bus_hold_flag)
+		if ((id->defeature_repeated_start && !id->send_count) ||
+		    (!id->send_count && !id->bus_hold_flag))
 			cdns_i2c_clear_bus_hold(id);
 
 		status = IRQ_HANDLED;
@@ -347,10 +350,15 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	} 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 &&
-		((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
-		(id->recv_count <= CDNS_I2C_FIFO_DEPTH))
-			cdns_i2c_clear_bus_hold(id);
+	if (id->defeature_repeated_start &&
+	    (((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
+	     (id->recv_count <= CDNS_I2C_FIFO_DEPTH)))
+		cdns_i2c_clear_bus_hold(id);
+	else if (!id->bus_hold_flag &&
+		 ((id->p_msg->flags & I2C_M_RECV_LEN) != I2C_M_RECV_LEN) &&
+		 (id->recv_count <= CDNS_I2C_FIFO_DEPTH))
+		cdns_i2c_clear_bus_hold(id);
+
 	/* Set the slave address in address register - triggers operation */
 	cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
 						CDNS_I2C_ADDR_OFFSET);
@@ -411,8 +419,10 @@ static void cdns_i2c_msend(struct cdns_i2c *id)
 	 * Clear the bus hold flag if there is no more data
 	 * and if it is the last message.
 	 */
-	if (!id->bus_hold_flag && !id->send_count)
+	 if ((id->defeature_repeated_start && !id->send_count) ||
+	     (!id->send_count && !id->bus_hold_flag))
 		cdns_i2c_clear_bus_hold(id);
+
 	/* Set the slave address in address register - triggers operation. */
 	cdns_i2c_writereg(id->p_msg->addr & CDNS_I2C_ADDR_MASK,
 						CDNS_I2C_ADDR_OFFSET);
@@ -520,19 +530,25 @@ static int cdns_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	/*
 	 * Set the flag to one when multiple messages are to be
 	 * processed with a repeated start.
+	 * De feature repeated start when defeature_repeated_start flag is set
 	 */
-	if (num > 1) {
-		id->bus_hold_flag = 1;
-		reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
-		reg |= CDNS_I2C_CR_HOLD;
-		cdns_i2c_writereg(reg, CDNS_I2C_CR_OFFSET);
+	if (!id->defeature_repeated_start && num > 1) {
+			for (count = 0; count < num-1; count++) {
+				if (msgs[count].flags & I2C_M_RD)
+					return -EINVAL;
+			}
+			id->bus_hold_flag = 1;
+			reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
+			reg |= CDNS_I2C_CR_HOLD;
+			cdns_i2c_writereg(reg, CDNS_I2C_CR_OFFSET);
 	} else {
 		id->bus_hold_flag = 0;
 	}
 
 	/* Process the msg one by one */
 	for (count = 0; count < num; count++, msgs++) {
-		if (count == (num - 1))
+		if (!id->defeature_repeated_start &&
+		    (count == (num - 1)))
 			id->bus_hold_flag = 0;
 
 		ret = cdns_i2c_process_msg(id, msgs, adap);
@@ -840,6 +856,8 @@ static int cdns_i2c_probe(struct platform_device *pdev)
 	cdns_i2c_writereg(CDNS_I2C_CR_ACK_EN | CDNS_I2C_CR_NEA | CDNS_I2C_CR_MS,
 			  CDNS_I2C_CR_OFFSET);
 
+	of_property_read_u32(pdev->dev.of_node,	"defeature-repeated-start",
+			     &id->defeature_repeated_start);
 	ret = cdns_i2c_setclk(id->input_clk, id);
 	if (ret) {
 		dev_err(&pdev->dev, "invalid SCL clock: %u Hz\n", id->i2c_clk);
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 4/4] i2c: cadence: Defeature repeated start based on devicetree property

[Mark Rutland]

On Tue, Dec 02, 2014 at 10:05:49AM +0000, Harini Katakam wrote:
> From: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> 
> This patch defeatures repeated start in the driver if
> "defeature-repeated-start" property is present in the devicetree.
> 
> This defeature is proposed due to a few bugs in the controller
> - 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.
> 
> Signed-off-by: Vishnu Motghare <vishnum-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Harini Katakam <harinik-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-cadence.c |   44 +++++++++++++++++++++++++++-----------
>  1 file changed, 31 insertions(+), 13 deletions(-)

[...]

> @@ -840,6 +856,8 @@ static int cdns_i2c_probe(struct platform_device *pdev)
>  	cdns_i2c_writereg(CDNS_I2C_CR_ACK_EN | CDNS_I2C_CR_NEA | CDNS_I2C_CR_MS,
>  			  CDNS_I2C_CR_OFFSET);
>  
> +	of_property_read_u32(pdev->dev.of_node,	"defeature-repeated-start",
> +			     &id->defeature_repeated_start);

This will not work with the binding you described. You want to treat
this as a bool, and use of_property_read_bool.

Thanks,
Mark.