[06/12] i2c: qup: proper error handling for i2c error in BAM mode

Message ID 1517644697-30806-7-git-send-email-absahu@codeaurora.org
State Superseded
Headers show
Series
  • Major code reorganization to make all i2c transfers working
Related show

Commit Message

Abhishek Sahu Feb. 3, 2018, 7:58 a.m.
Currently the i2c error handling in BAM mode is not working
properly in stress condition.

1. After an error, the FIFO are being written with FLUSH and
   EOT tags which should not be required since already these tags
   have been written in BAM descriptor itself.

2. QUP state is being moved to RESET in IRQ handler in case
   of error. When QUP HW encounters an error in BAM mode then it
   moves the QUP STATE to PAUSE state. In this case, I2C_FLUSH
   command needs to be executed while moving to RUN_STATE by writing
   to the QUP_STATE register with the I2C_FLUSH bit set to 1.

3. In Error case, sometimes, QUP generates more than one
   interrupt which will trigger the complete again. After an error,
   the flush operation will be scheduled after doing
   reinit_completion which should be triggered by BAM IRQ callback.
   If the second QUP IRQ comes during this time then it will call
   the complete and the transfer function will assume the all the
   BAM HW descriptors have been completed.

4. The release DMA is being called after each error which
   will free the DMA tx and rx channels. The error like NACK is very
   common in I2C transfer and every time this will be overhead. Now,
   since the error handling is proper so this release channel can be
   completely avoided.

Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
---
 drivers/i2c/busses/i2c-qup.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

Comments

Sricharan R Feb. 16, 2018, 4:33 a.m. | #1
On 2/3/2018 1:28 PM, Abhishek Sahu wrote:
> Currently the i2c error handling in BAM mode is not working
> properly in stress condition.
> 
> 1. After an error, the FIFO are being written with FLUSH and
>    EOT tags which should not be required since already these tags
>    have been written in BAM descriptor itself.
> 
> 2. QUP state is being moved to RESET in IRQ handler in case
>    of error. When QUP HW encounters an error in BAM mode then it
>    moves the QUP STATE to PAUSE state. In this case, I2C_FLUSH
>    command needs to be executed while moving to RUN_STATE by writing
>    to the QUP_STATE register with the I2C_FLUSH bit set to 1.
> 
> 3. In Error case, sometimes, QUP generates more than one
>    interrupt which will trigger the complete again. After an error,
>    the flush operation will be scheduled after doing
>    reinit_completion which should be triggered by BAM IRQ callback.
>    If the second QUP IRQ comes during this time then it will call
>    the complete and the transfer function will assume the all the
>    BAM HW descriptors have been completed.
> 
> 4. The release DMA is being called after each error which
>    will free the DMA tx and rx channels. The error like NACK is very
>    common in I2C transfer and every time this will be overhead. Now,
>    since the error handling is proper so this release channel can be
>    completely avoided.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
>  drivers/i2c/busses/i2c-qup.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index 094be6a..6227a5c 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -228,9 +228,24 @@ static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
>  	if (bus_err)
>  		writel(bus_err, qup->base + QUP_I2C_STATUS);
>  
> +	/*
> +	 * Check for BAM mode and returns if already error has come for current
> +	 * transfer. In Error case, sometimes, QUP generates more than one
> +	 * interrupt.
> +	 */
> +	if (qup->use_dma && (qup->qup_err || qup->bus_err))
> +		return IRQ_HANDLED;
> +
>  	/* Reset the QUP State in case of error */
>  	if (qup_err || bus_err) {
> -		writel(QUP_RESET_STATE, qup->base + QUP_STATE);
> +		/*
> +		 * Don’t reset the QUP state in case of BAM mode. The BAM
> +		 * flush operation needs to be scheduled in transfer function
> +		 * which will clear the remaining schedule descriptors in BAM
> +		 * HW FIFO and generates the BAM interrupt.
> +		 */
> +		if (!qup->use_dma)
> +			writel(QUP_RESET_STATE, qup->base + QUP_STATE);
>  		goto done;
>  	}
>  
> @@ -841,20 +856,12 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
>  			goto desc_err;
>  		}
>  
> -		if (rx_buf)
> -			writel(QUP_BAM_INPUT_EOT,
> -			       qup->base + QUP_OUT_FIFO_BASE);
> -
> -		writel(QUP_BAM_FLUSH_STOP, qup->base + QUP_OUT_FIFO_BASE);
> -
>  		qup_i2c_flush(qup);
>  
>  		/* wait for remaining interrupts to occur */
>  		if (!wait_for_completion_timeout(&qup->xfer, HZ))
>  			dev_err(qup->dev, "flush timed out\n");
>  
> -		qup_i2c_rel_dma(qup);
> -
>  		ret =  (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
>  	}
>  
> 

 Reviewed-by: Sricharan R <sricharan@codeaurora.org>

Regards,
 Sricharan
Christ, Austin Feb. 27, 2018, 10 p.m. | #2
Tested on Centriq 2400

Reviewed-by: Austin Christ <austinwc@codeaurora.org>

On 2/3/2018 12:58 AM, Abhishek Sahu wrote:
> Currently the i2c error handling in BAM mode is not working
> properly in stress condition.
> 
> 1. After an error, the FIFO are being written with FLUSH and
>     EOT tags which should not be required since already these tags
>     have been written in BAM descriptor itself.
> 
> 2. QUP state is being moved to RESET in IRQ handler in case
>     of error. When QUP HW encounters an error in BAM mode then it
>     moves the QUP STATE to PAUSE state. In this case, I2C_FLUSH
>     command needs to be executed while moving to RUN_STATE by writing
>     to the QUP_STATE register with the I2C_FLUSH bit set to 1.
> 
> 3. In Error case, sometimes, QUP generates more than one
>     interrupt which will trigger the complete again. After an error,
>     the flush operation will be scheduled after doing
>     reinit_completion which should be triggered by BAM IRQ callback.
>     If the second QUP IRQ comes during this time then it will call
>     the complete and the transfer function will assume the all the
>     BAM HW descriptors have been completed.
> 
> 4. The release DMA is being called after each error which
>     will free the DMA tx and rx channels. The error like NACK is very
>     common in I2C transfer and every time this will be overhead. Now,
>     since the error handling is proper so this release channel can be
>     completely avoided.
> 
> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org>
> ---
>   drivers/i2c/busses/i2c-qup.c | 25 ++++++++++++++++---------
>   1 file changed, 16 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index 094be6a..6227a5c 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -228,9 +228,24 @@ static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
>   	if (bus_err)
>   		writel(bus_err, qup->base + QUP_I2C_STATUS);
>   
> +	/*
> +	 * Check for BAM mode and returns if already error has come for current
> +	 * transfer. In Error case, sometimes, QUP generates more than one
> +	 * interrupt.
> +	 */
> +	if (qup->use_dma && (qup->qup_err || qup->bus_err))
> +		return IRQ_HANDLED;
> +
>   	/* Reset the QUP State in case of error */
>   	if (qup_err || bus_err) {
> -		writel(QUP_RESET_STATE, qup->base + QUP_STATE);
> +		/*
> +		 * Don’t reset the QUP state in case of BAM mode. The BAM
> +		 * flush operation needs to be scheduled in transfer function
> +		 * which will clear the remaining schedule descriptors in BAM
> +		 * HW FIFO and generates the BAM interrupt.
> +		 */
> +		if (!qup->use_dma)
> +			writel(QUP_RESET_STATE, qup->base + QUP_STATE);
>   		goto done;
>   	}
>   
> @@ -841,20 +856,12 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
>   			goto desc_err;
>   		}
>   
> -		if (rx_buf)
> -			writel(QUP_BAM_INPUT_EOT,
> -			       qup->base + QUP_OUT_FIFO_BASE);
> -
> -		writel(QUP_BAM_FLUSH_STOP, qup->base + QUP_OUT_FIFO_BASE);
> -
>   		qup_i2c_flush(qup);
>   
>   		/* wait for remaining interrupts to occur */
>   		if (!wait_for_completion_timeout(&qup->xfer, HZ))
>   			dev_err(qup->dev, "flush timed out\n");
>   
> -		qup_i2c_rel_dma(qup);
> -
>   		ret =  (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
>   	}
>   
>
Andy Gross Feb. 27, 2018, 10:58 p.m. | #3
On Sat, Feb 03, 2018 at 01:28:11PM +0530, Abhishek Sahu wrote:

<snip>

> @@ -841,20 +856,12 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
>  			goto desc_err;
>  		}
>  
> -		if (rx_buf)
> -			writel(QUP_BAM_INPUT_EOT,
> -			       qup->base + QUP_OUT_FIFO_BASE);
> -
> -		writel(QUP_BAM_FLUSH_STOP, qup->base + QUP_OUT_FIFO_BASE);
> -
>  		qup_i2c_flush(qup);
>  
>  		/* wait for remaining interrupts to occur */
>  		if (!wait_for_completion_timeout(&qup->xfer, HZ))
>  			dev_err(qup->dev, "flush timed out\n");
>  
> -		qup_i2c_rel_dma(qup);
> -

So this really only works due to the previous patch that adds the flush/eot tags
to all of the read messages.  If the answer to the previous question is that
only the last read message gets the eot/flush, then this code needs to remain in
place.  Otherwise, it's fine.


Andy
Abhishek Sahu March 12, 2018, 12:34 p.m. | #4
On 2018-02-28 04:28, Andy Gross wrote:
> On Sat, Feb 03, 2018 at 01:28:11PM +0530, Abhishek Sahu wrote:
> 
> <snip>
> 
>> @@ -841,20 +856,12 @@ static int qup_i2c_bam_do_xfer(struct 
>> qup_i2c_dev *qup, struct i2c_msg *msg,
>>  			goto desc_err;
>>  		}
>> 
>> -		if (rx_buf)
>> -			writel(QUP_BAM_INPUT_EOT,
>> -			       qup->base + QUP_OUT_FIFO_BASE);
>> -
>> -		writel(QUP_BAM_FLUSH_STOP, qup->base + QUP_OUT_FIFO_BASE);
>> -
>>  		qup_i2c_flush(qup);
>> 
>>  		/* wait for remaining interrupts to occur */
>>  		if (!wait_for_completion_timeout(&qup->xfer, HZ))
>>  			dev_err(qup->dev, "flush timed out\n");
>> 
>> -		qup_i2c_rel_dma(qup);
>> -
> 
> So this really only works due to the previous patch that adds the 
> flush/eot tags
> to all of the read messages.  If the answer to the previous question is 
> that
> only the last read message gets the eot/flush, then this code needs to 
> remain in
> place.  Otherwise, it's fine.
> 
> 
> Andy

   Thanks Andy,

   We need to schedule EOT/FLUSH after last message.
   For following transfer

   READ, READ, WRITE (FLUSH + EOT tags after that)

   In this case, FLUSH will clear all the descriptors till WRITE and 
trigger TX
   completion. EOT will be copied in RX FIFO and trigger RX completion.

   Thanks,
   Abhishek

Patch

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 094be6a..6227a5c 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -228,9 +228,24 @@  static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
 	if (bus_err)
 		writel(bus_err, qup->base + QUP_I2C_STATUS);
 
+	/*
+	 * Check for BAM mode and returns if already error has come for current
+	 * transfer. In Error case, sometimes, QUP generates more than one
+	 * interrupt.
+	 */
+	if (qup->use_dma && (qup->qup_err || qup->bus_err))
+		return IRQ_HANDLED;
+
 	/* Reset the QUP State in case of error */
 	if (qup_err || bus_err) {
-		writel(QUP_RESET_STATE, qup->base + QUP_STATE);
+		/*
+		 * Don’t reset the QUP state in case of BAM mode. The BAM
+		 * flush operation needs to be scheduled in transfer function
+		 * which will clear the remaining schedule descriptors in BAM
+		 * HW FIFO and generates the BAM interrupt.
+		 */
+		if (!qup->use_dma)
+			writel(QUP_RESET_STATE, qup->base + QUP_STATE);
 		goto done;
 	}
 
@@ -841,20 +856,12 @@  static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 			goto desc_err;
 		}
 
-		if (rx_buf)
-			writel(QUP_BAM_INPUT_EOT,
-			       qup->base + QUP_OUT_FIFO_BASE);
-
-		writel(QUP_BAM_FLUSH_STOP, qup->base + QUP_OUT_FIFO_BASE);
-
 		qup_i2c_flush(qup);
 
 		/* wait for remaining interrupts to occur */
 		if (!wait_for_completion_timeout(&qup->xfer, HZ))
 			dev_err(qup->dev, "flush timed out\n");
 
-		qup_i2c_rel_dma(qup);
-
 		ret =  (qup->bus_err & QUP_I2C_NACK_FLAG) ? -ENXIO : -EIO;
 	}