Major code reorganization to make all i2c transfers working

The QUP BSLP BAM generates the following error sometimes if the current I2C DMA transfer fails and the flush operation has been scheduled

“bam-dma-engine 7884000.dma: Cannot free busy channel”

If any I2C error comes during BAM DMA transfer, then the QUP I2C interrupt will be generated and the flush operation will be carried out to make i2c consume all scheduled DMA transfer. Currently, the same completion structure is being used for BAM transfer which has already completed without reinit. It will make flush operation wait_for_completion_timeout completed immediately and will proceed for freeing the DMA resources where the descriptors are still in process.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-qup.c +0 -0

index 08f8e01..9faa26c41a 100644

Hi Abhishek,

Quoted 20 lines · Abhishek Sahu

The QUP BSLP BAM generates the following error sometimes if the current I2C DMA transfer fails and the flush operation has been scheduled

“bam-dma-engine 7884000.dma: Cannot free busy channel”

If any I2C error comes during BAM DMA transfer, then the QUP I2C interrupt will be generated and the flush operation will be carried out to make i2c consume all scheduled DMA transfer. Currently, the same completion structure is being used for BAM transfer which has already completed without reinit. It will make flush operation wait_for_completion_timeout completed immediately and will proceed for freeing the DMA resources where the descriptors are still in process.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Quoted 10 lines

drivers/i2c/busses/i2c-qup.c +0 -1

index 08f8e01..9faa26c41a 100644
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009-2013, The Linux Foundation. All rights 

reserved.
+ * Copyright (c) 2009-2013, 2016-2018, The Linux Foundation. All rights reserved.

Not sure, if this was an intended change. But given that you are fixing this,

you can change the header to the new SPDX one.

Quoted 12 lines

* Copyright (c) 2014, Sony Mobile Communications AB. *
*

drivers/i2c/busses/i2c-qup.c +2 -0

@@ -844,6 +844,8 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev 
 *qup, struct i2c_msg *msg,
 	}
 
 	if (ret || qup->bus_err || qup->qup_err) {
+		reinit_completion(&qup->xfer);
+
 		if (qup_i2c_change_state(qup, QUP_RUN_STATE)) {
 			dev_err(qup->dev, "change to run state timed out");

Quoted 2 lines

goto desc_err;

Except for the above nit,

Acked-by Sricharan R <[email protected]>

Regards,
Sricharan

The QUP BSLP BAM generates the following error sometimes if the current I2C DMA transfer fails and the flush operation has been scheduled

“bam-dma-engine 7884000.dma: Cannot free busy channel”

If any I2C error comes during BAM DMA transfer, then the QUP I2C interrupt will be generated and the flush operation will be carried out to make i2c consume all scheduled DMA transfer. Currently, the same completion structure is being used for BAM transfer which has already completed without reinit. It will make flush operation wait_for_completion_timeout completed immediately and will proceed for freeing the DMA resources where the descriptors are still in process.

Signed-off-by Abhishek Sahu <[email protected]>

---
  drivers/i2c/busses/i2c-qup.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-qup.c +3 -1

index 08f8e01..9faa26c41a 100644
@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2009-2013, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2009-2013, 2016-2018, The Linux Foundation. All rights reserved.
   * Copyright (c) 2014, Sony Mobile Communications AB.
   *
   *

@@ -844,6 +844,8 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  	}
 
  	if (ret || qup->bus_err || qup->qup_err) {
+		reinit_completion(&qup->xfer);
+
  		if (qup_i2c_change_state(qup, QUP_RUN_STATE)) {
  			dev_err(qup->dev, "change to run state timed out");
  			goto desc_err;

The QUP BSLP BAM generates the following error sometimes if the current I2C DMA transfer fails and the flush operation has been scheduled

“bam-dma-engine 7884000.dma: Cannot free busy channel”

If any I2C error comes during BAM DMA transfer, then the QUP I2C interrupt will be generated and the flush operation will be carried out to make i2c consume all scheduled DMA transfer. Currently, the same completion structure is being used for BAM transfer which has already completed without reinit. It will make flush operation wait_for_completion_timeout completed immediately and will proceed for freeing the DMA resources where the descriptors are still in process.

Signed-off-by Abhishek Sahu <[email protected]>

1. Assigns use_dma in qup_dev structure itself which will help in subsequent patches to determine the mode in IRQ handler.
2. Does minor code reorganization for loops to reduce the unnecessary comparison and assignment.

Signed-off-by Abhishek Sahu <[email protected]>

---
  drivers/i2c/busses/i2c-qup.c | 19 +++++++++++--------
  1 file changed, 11 insertions(+), 8 deletions(-)

drivers/i2c/busses/i2c-qup.c +11 -8

index 9faa26c41a..c68f433 100644
@@ -190,6 +190,8 @@ struct qup_i2c_dev {
 
  	/* dma parameters */
  	bool			is_dma;
+	/* To check if the current transfer is using DMA */
+	bool			use_dma;
  	struct			dma_pool *dpool;
  	struct			qup_i2c_tag start_tag;
  	struct			qup_i2c_bam brx;

@@ -1297,7 +1299,7 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
  			   int num)
  {
  	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
-	int ret, len, idx = 0, use_dma = 0;
+	int ret, len, idx = 0;
 
  	qup->bus_err = 0;
  	qup->qup_err = 0;

@@ -1326,13 +1328,12 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
  			len = (msgs[idx].len > qup->out_fifo_sz) ||
  			      (msgs[idx].len > qup->in_fifo_sz);
 
-			if ((!is_vmalloc_addr(msgs[idx].buf)) && len) {
-				use_dma = 1;
-			 } else {
-				use_dma = 0;
+			if (is_vmalloc_addr(msgs[idx].buf) || !len)
  				break;
-			}
  		}
+
+		if (idx == num)
+			qup->use_dma = true;
  	}
 
  	idx = 0;

@@ -1356,15 +1357,17 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
 
  		reinit_completion(&qup->xfer);
 
-		if (use_dma) {
+		if (qup->use_dma) {
  			ret = qup_i2c_bam_xfer(adap, &msgs[idx], num);
+			qup->use_dma = false;
+			break;
  		} else {
  			if (msgs[idx].flags & I2C_M_RD)
  				ret = qup_i2c_read_one_v2(qup, &msgs[idx]);
  			else
  				ret = qup_i2c_write_one_v2(qup, &msgs[idx]);
  		}
-	} while ((idx++ < (num - 1)) && !use_dma && !ret);
+	} while ((idx++ < (num - 1)) && !ret);
 
  	if (!ret)
  		ret = qup_i2c_change_state(qup, QUP_RESET_STATE);

1. Assigns use_dma in qup_dev structure itself which will help in subsequent patches to determine the mode in IRQ handler.
2. Does minor code reorganization for loops to reduce the unnecessary comparison and assignment.

Signed-off-by Abhishek Sahu <[email protected]>

The rx_nents and tx_nents are redundant. rx_buf and tx_buf can be used for total number of SG entries.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/i2c/busses/i2c-qup.c +0 -0

index c68f433..bb83a2967 100644

Hi Abhishek,

Quoted 17 lines · Abhishek Sahu

The rx_nents and tx_nents are redundant. rx_buf and tx_buf can be used for total number of SG entries.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

drivers/i2c/busses/i2c-qup.c +0 -0

index c68f433..bb83a2967 100644
@@ -692,7 +692,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev 
 *qup, struct i2c_msg *msg,
 	struct dma_async_tx_descriptor *txd, *rxd = NULL;
 	int ret = 0, idx = 0, limit = QUP_READ_LIMIT;
 	dma_cookie_t cookie_rx, cookie_tx;

drivers/i2c/busses/i2c-qup.c static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg, · line 697

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c +1 -1

-	u32 rx_nents = 0, tx_nents = 0, len, blocks, rem;
+	u32 len, blocks, rem;
 	u32 i, tlen, tx_len, tx_buf = 0, rx_buf = 0, off = 0;
 	u8 *tags;

This is correct. Just a nit, may be rx/tx_buf can be changed to rx/tx_count to make it more clear.

The rx_nents and tx_nents are redundant. rx_buf and tx_buf can be used for total number of SG entries.

Signed-off-by Abhishek Sahu <[email protected]>

---
  drivers/i2c/busses/i2c-qup.c | 26 ++++++++++----------------
  1 file changed, 10 insertions(+), 16 deletions(-)

drivers/i2c/busses/i2c-qup.c +10 -16

index c68f433..bb83a2967 100644
@@ -692,7 +692,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  	struct dma_async_tx_descriptor *txd, *rxd = NULL;
  	int ret = 0, idx = 0, limit = QUP_READ_LIMIT;
  	dma_cookie_t cookie_rx, cookie_tx;
-	u32 rx_nents = 0, tx_nents = 0, len, blocks, rem;
+	u32 len, blocks, rem;
  	u32 i, tlen, tx_len, tx_buf = 0, rx_buf = 0, off = 0;
  	u8 *tags;
 

@@ -707,9 +707,6 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  		rem = msg->len - (blocks - 1) * limit;
 
  		if (msg->flags & I2C_M_RD) {
-			rx_nents += (blocks * 2) + 1;
-			tx_nents += 1;
-
  			while (qup->blk.pos < blocks) {
  				tlen = (i == (blocks - 1)) ? rem : limit;
  				tags = &qup->start_tag.start[off + len];

@@ -748,8 +745,6 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  			if (ret)
  				return ret;
  		} else {
-			tx_nents += (blocks * 2);
-
  			while (qup->blk.pos < blocks) {
  				tlen = (i == (blocks - 1)) ? rem : limit;
  				tags = &qup->start_tag.start[off + tx_len];

@@ -775,7 +770,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 
  			if (idx == (num - 1)) {
  				len = 1;
-				if (rx_nents) {
+				if (rx_buf) {
  					qup->btx.tag.start[0] =
  							QUP_BAM_INPUT_EOT;
  					len++;

@@ -787,14 +782,13 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  						     len, qup, DMA_TO_DEVICE);
  				if (ret)
  					return ret;
-				tx_nents += 1;
  			}
  		}
  		idx++;
  		msg++;
  	}
 
-	txd = dmaengine_prep_slave_sg(qup->btx.dma, qup->btx.sg, tx_nents,
+	txd = dmaengine_prep_slave_sg(qup->btx.dma, qup->btx.sg, tx_buf,
  				      DMA_MEM_TO_DEV,
  				      DMA_PREP_INTERRUPT | DMA_PREP_FENCE);
  	if (!txd) {

@@ -803,7 +797,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  		goto desc_err;
  	}
 
-	if (!rx_nents) {
+	if (!rx_buf) {
  		txd->callback = qup_i2c_bam_cb;
  		txd->callback_param = qup;
  	}

@@ -816,9 +810,9 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 
  	dma_async_issue_pending(qup->btx.dma);
 
-	if (rx_nents) {
+	if (rx_buf) {
  		rxd = dmaengine_prep_slave_sg(qup->brx.dma, qup->brx.sg,
-					      rx_nents, DMA_DEV_TO_MEM,
+					      rx_buf, DMA_DEV_TO_MEM,
  					      DMA_PREP_INTERRUPT);
  		if (!rxd) {
  			dev_err(qup->dev, "failed to get rx desc\n");

@@ -853,7 +847,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  			goto desc_err;
  		}
 
-		if (rx_nents)
+		if (rx_buf)
  			writel(QUP_BAM_INPUT_EOT,
  			       qup->base + QUP_OUT_FIFO_BASE);
 

@@ -871,10 +865,10 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  	}
 
  desc_err:
-	dma_unmap_sg(qup->dev, qup->btx.sg, tx_nents, DMA_TO_DEVICE);
+	dma_unmap_sg(qup->dev, qup->btx.sg, tx_buf, DMA_TO_DEVICE);
 
-	if (rx_nents)
-		dma_unmap_sg(qup->dev, qup->brx.sg, rx_nents,
+	if (rx_buf)
+		dma_unmap_sg(qup->dev, qup->brx.sg, rx_buf,
  			     DMA_FROM_DEVICE);
 
  	return ret;

The rx_nents and tx_nents are redundant. rx_buf and tx_buf can be used for total number of SG entries.

Signed-off-by Abhishek Sahu <[email protected]>

---

A single BAM transfer can have multiple read and write messages. The EOT and FLUSH tags should be scheduled at the end of BAM HW descriptors. Since the READ and WRITE can be present in any order so for some of the cases, these tags are not being written correctly.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 54 ++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

drivers/i2c/busses/i2c-qup.c +4 -24

index bb83a2967..6357aff 100644
@@ -560,7 +560,7 @@ static int qup_i2c_set_tags_smb(u16 addr, u8 *tags, struct qup_i2c_dev *qup,
 }
 
 static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
-			    struct i2c_msg *msg,  int is_dma)
+			    struct i2c_msg *msg)
 {
 	u16 addr = i2c_8bit_addr_from_msg(msg);
 	int len = 0;

@@ -601,11 +601,6 @@ static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
 	else
 		tags[len++] = data_len;
 
-	if ((msg->flags & I2C_M_RD) && last && is_dma) {
-		tags[len++] = QUP_BAM_INPUT_EOT;
-		tags[len++] = QUP_BAM_FLUSH_STOP;
-	}
-
 	return len;
 }
 

@@ -614,7 +609,7 @@ static int qup_i2c_issue_xfer_v2(struct qup_i2c_dev *qup, struct i2c_msg *msg)
 	int data_len = 0, tag_len, index;
 	int ret;
 
-	tag_len = qup_i2c_set_tags(qup->blk.tags, qup, msg, 0);
+	tag_len = qup_i2c_set_tags(qup->blk.tags, qup, msg);
 	index = msg->len - qup->blk.data_len;
 
 	/* only tags are written for read */

@@ -710,7 +705,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 			while (qup->blk.pos < blocks) {
 				tlen = (i == (blocks - 1)) ? rem : limit;
 				tags = &qup->start_tag.start[off + len];
-				len += qup_i2c_set_tags(tags, qup, msg, 1);
+				len += qup_i2c_set_tags(tags, qup, msg);
 				qup->blk.data_len -= tlen;
 
 				/* scratch buf to read the start and len tags */

@@ -738,17 +733,11 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 				return ret;
 
 			off += len;
-			/* scratch buf to read the BAM EOT and FLUSH tags */
-			ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
-					     &qup->brx.tag.start[0],
-					     2, qup, DMA_FROM_DEVICE);
-			if (ret)
-				return ret;
 		} else {
 			while (qup->blk.pos < blocks) {
 				tlen = (i == (blocks - 1)) ? rem : limit;
 				tags = &qup->start_tag.start[off + tx_len];
-				len = qup_i2c_set_tags(tags, qup, msg, 1);
+				len = qup_i2c_set_tags(tags, qup, msg);
 				qup->blk.data_len -= tlen;
 
 				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],

@@ -768,26 +757,31 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 			}
 			off += tx_len;
 
-			if (idx == (num - 1)) {
-				len = 1;
-				if (rx_buf) {
-					qup->btx.tag.start[0] =
-							QUP_BAM_INPUT_EOT;
-					len++;
-				}
-				qup->btx.tag.start[len - 1] =
-							QUP_BAM_FLUSH_STOP;

drivers/i2c/busses/i2c-qup.c +10 -0

+	/* schedule the EOT and FLUSH I2C tags */
+	len = 1;
+	if (rx_buf) {
+		qup->btx.tag.start[0] = QUP_BAM_INPUT_EOT;
+		len++;
+
+		/* scratch buf to read the BAM EOT and FLUSH tags */
+		ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
+				     &qup->brx.tag.start[0],
+				     2, qup, DMA_FROM_DEVICE);

Hi Abhishek,

Quoted 101 lines · Abhishek Sahu

A single BAM transfer can have multiple read and write messages. The EOT and FLUSH tags should be scheduled at the end of BAM HW descriptors. Since the READ and WRITE can be present in any order so for some of the cases, these tags are not being written correctly.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 54 
++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

drivers/i2c/busses/i2c-qup.c +1 -1

index bb83a2967..6357aff 100644
@@ -560,7 +560,7 @@ static int qup_i2c_set_tags_smb(u16 addr, u8 
 *tags, struct qup_i2c_dev *qup,
 }
 
 static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
-			    struct i2c_msg *msg,  int is_dma)
+			    struct i2c_msg *msg)
 {
 	u16 addr = i2c_8bit_addr_from_msg(msg);

drivers/i2c/busses/i2c-qup.c +1 -6

 	int len = 0;

@@ -601,11 +601,6 @@ static int qup_i2c_set_tags(u8 *tags, struct 
 qup_i2c_dev *qup,
 	else
 		tags[len++] = data_len;
 
-	if ((msg->flags & I2C_M_RD) && last && is_dma) {
-		tags[len++] = QUP_BAM_INPUT_EOT;
-		tags[len++] = QUP_BAM_FLUSH_STOP;
-	}
-
 	return len;
 }

@@ -614,7 +609,7 @@ static int qup_i2c_issue_xfer_v2(struct 
 qup_i2c_dev *qup, struct i2c_msg *msg)
 	int data_len = 0, tag_len, index;
 	int ret;
 
-	tag_len = qup_i2c_set_tags(qup->blk.tags, qup, msg, 0);
+	tag_len = qup_i2c_set_tags(qup->blk.tags, qup, msg);
 	index = msg->len - qup->blk.data_len;
 

drivers/i2c/busses/i2c-qup.c +1 -1

 	/* only tags are written for read */

@@ -710,7 +705,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev 
 *qup, struct i2c_msg *msg,
 			while (qup->blk.pos < blocks) {
 				tlen = (i == (blocks - 1)) ? rem : limit;
 				tags = &qup->start_tag.start[off + len];
-				len += qup_i2c_set_tags(tags, qup, msg, 1);
+				len += qup_i2c_set_tags(tags, qup, msg);
 				qup->blk.data_len -= tlen;
 

drivers/i2c/busses/i2c-qup.c +1 -7

 				/* scratch buf to read the start and len tags */

@@ -738,17 +733,11 @@ static int qup_i2c_bam_do_xfer(struct 
 qup_i2c_dev *qup, struct i2c_msg *msg,
 				return ret;
 
 			off += len;
-			/* scratch buf to read the BAM EOT and FLUSH tags */
-			ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
-					     &qup->brx.tag.start[0],
-					     2, qup, DMA_FROM_DEVICE);
-			if (ret)
-				return ret;
 		} else {
 			while (qup->blk.pos < blocks) {
 				tlen = (i == (blocks - 1)) ? rem : limit;
 				tags = &qup->start_tag.start[off + tx_len];
-				len = qup_i2c_set_tags(tags, qup, msg, 1);
+				len = qup_i2c_set_tags(tags, qup, msg);
 				qup->blk.data_len -= tlen;
 

drivers/i2c/busses/i2c-qup.c +0 -15

 				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],

@@ -768,26 +757,31 @@ static int qup_i2c_bam_do_xfer(struct 
 qup_i2c_dev *qup, struct i2c_msg *msg,
 			}
 			off += tx_len;
 
-			if (idx == (num - 1)) {
-				len = 1;
-				if (rx_buf) {
-					qup->btx.tag.start[0] =
-							QUP_BAM_INPUT_EOT;
-					len++;
-				}
-				qup->btx.tag.start[len - 1] =
-							QUP_BAM_FLUSH_STOP;
-				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
-						     &qup->btx.tag.start[0],
-						     len, qup, DMA_TO_DEVICE);
-				if (ret)
-					return ret;
-			}
 		}
 		idx++;

drivers/i2c/busses/i2c-qup.c static int qup_i2c_bam_do_xfer(struct · line 764

Quoted 2 lines

drivers/i2c/busses/i2c-qup.c +0 -0

 		msg++;
 	}

While here, can you please split above the if, else in to two
separate functions ?
and probably one more function for handling the NACK case down below.
The function
size at the moment is too big.

Quoted 19 lines

drivers/i2c/busses/i2c-qup.c +18 -0

+	/* schedule the EOT and FLUSH I2C tags */
+	len = 1;
+	if (rx_buf) {
+		qup->btx.tag.start[0] = QUP_BAM_INPUT_EOT;
+		len++;
+
+		/* scratch buf to read the BAM EOT and FLUSH tags */
+		ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
+				     &qup->brx.tag.start[0],
+				     2, qup, DMA_FROM_DEVICE);
+		if (ret)
+			return ret;
+	}
+
+	qup->btx.tag.start[len - 1] = QUP_BAM_FLUSH_STOP;
+	ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++], &qup->btx.tag.start[0],
+			     len, qup, DMA_TO_DEVICE);
+	if (ret)

drivers/i2c/busses/i2c-qup.c static int qup_i2c_bam_do_xfer(struct · line 784

Quoted 2 lines

drivers/i2c/busses/i2c-qup.c +2 -0

+		return ret;
+

May be you can change the commit to make it explicit to say what is
being
fixed, like "current code is broken when there is sequence of
wr,rd,wr" like that.
Agree on this fix otherwise.

A single BAM transfer can have multiple read and write messages. The EOT and FLUSH tags should be scheduled at the end of BAM HW descriptors. Since the READ and WRITE can be present in any order so for some of the cases, these tags are not being written correctly.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 54 ++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

drivers/i2c/busses/i2c-qup.c +1 -1

index bb83a2967..6357aff 100644
@@ -560,7 +560,7 @@ static int qup_i2c_set_tags_smb(u16 addr, u8 *tags, struct qup_i2c_dev *qup,
 }
 
 static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
-			    struct i2c_msg *msg,  int is_dma)
+			    struct i2c_msg *msg)
 {
 	u16 addr = i2c_8bit_addr_from_msg(msg);
 	int len = 0;

Quoted 31 lines · Abhishek Sahu

A single BAM transfer can have multiple read and write messages. The EOT and FLUSH tags should be scheduled at the end of BAM HW descriptors. Since the READ and WRITE can be present in any order so for some of the cases, these tags are not being written correctly.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 54 
++++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 30 deletions(-)

drivers/i2c/busses/i2c-qup.c +1 -1

index bb83a2967..6357aff 100644
@@ -560,7 +560,7 @@ static int qup_i2c_set_tags_smb(u16 addr, u8 
 *tags, struct qup_i2c_dev *qup,
 }
 
 static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
-			    struct i2c_msg *msg,  int is_dma)
+			    struct i2c_msg *msg)
 {
 	u16 addr = i2c_8bit_addr_from_msg(msg);

drivers/i2c/busses/i2c-qup.c +0 -0

 	int len = 0;

@@ -601,11 +601,6 @@ static int qup_i2c_set_tags(u8 *tags, struct 
 qup_i2c_dev *qup,
 	else
 		tags[len++] = data_len;
 

drivers/i2c/busses/i2c-qup.c static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup, · line 608

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c +0 -5

-	if ((msg->flags & I2C_M_RD) && last && is_dma) {
-		tags[len++] = QUP_BAM_INPUT_EOT;
-		tags[len++] = QUP_BAM_FLUSH_STOP;
-	}
-

So lets say you have multiple read and 1 write message. These changes will send a EOT/FLUSH for all reads. I think the intent here was that the last read message (not the last message) would have the EOT+FLUSH. Can there be an issue with sending EOT/FLUSH for all reads? And how does this mesh up with the BAM signaling?

In case of FLUSH operation, BAM copies INPUT EOT FLUSH (0x94) instead of normal EOT (0x93) tag in input data stream when an input EOT tag is received during flush operation. So only one tag will be written instead of 2 separate tags.

Signed-off-by Abhishek Sahu <[email protected]>

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 <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

drivers/i2c/busses/i2c-qup.c +16 -7

index 094be6a..6227a5c 100644
@@ -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);

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 <[email protected]>

---
  drivers/i2c/busses/i2c-qup.c | 25 ++++++++++++++++---------
  1 file changed, 16 insertions(+), 9 deletions(-)

drivers/i2c/busses/i2c-qup.c +16 -9

index 094be6a..6227a5c 100644
@@ -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;
  	}
 

drivers/i2c/busses/i2c-qup.c +0 -4

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

On Sat, Feb 03, 2018 at 01:28:11PM +0530, Abhishek Sahu wrote:

<snip>

Quoted 9 lines

drivers/i2c/busses/i2c-qup.c +0 -4

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

drivers/i2c/busses/i2c-qup.c static int qup_i2c_bam_do_xfer(struct · line 858

Quoted 10 lines

drivers/i2c/busses/i2c-qup.c +0 -4

-		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

Currently each message length in complete transfer is being checked for determining DMA mode and if any of the message length is less than FIFO length then non DMA mode is being used which will increase overhead. DMA can be used for any length and it should be determined with complete transfer length. Now, this patch introduces DMA threshold length and the transfer will be done in DMA mode if the total length is greater than this threshold length.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

drivers/i2c/busses/i2c-qup.c +8 -6

index 6227a5c..a91fc70 100644
@@ -192,6 +192,8 @@ struct qup_i2c_dev {
 	bool			is_dma;
 	/* To check if the current transfer is using DMA */
 	bool			use_dma;
+	/* The threshold length above which DMA will be used */
+	unsigned int		dma_threshold;
 	struct			dma_pool *dpool;
 	struct			qup_i2c_tag start_tag;
 	struct			qup_i2c_bam brx;

@@ -1294,7 +1296,8 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
 			   int num)
 {
 	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
-	int ret, len, idx = 0;
+	int ret, idx = 0;
+	unsigned int total_len = 0;
 
 	qup->bus_err = 0;
 	qup->qup_err = 0;

@@ -1320,14 +1323,13 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
 				goto out;
 			}
 
-			len = (msgs[idx].len > qup->out_fifo_sz) ||
-			      (msgs[idx].len > qup->in_fifo_sz);
-
-			if (is_vmalloc_addr(msgs[idx].buf) || !len)
+			if (is_vmalloc_addr(msgs[idx].buf))
 				break;
+
+			total_len += msgs[idx].len;
 		}
 
-		if (idx == num)
+		if (idx == num && total_len > qup->dma_threshold)
 			qup->use_dma = true;
 	}
 

Quoted 61 lines · Abhishek Sahu

Currently each message length in complete transfer is being checked for determining DMA mode and if any of the message length is less than FIFO length then non DMA mode is being used which will increase overhead. DMA can be used for any length and it should be determined with complete transfer length. Now, this patch introduces DMA threshold length and the transfer will be done in DMA mode if the total length is greater than this threshold length.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

drivers/i2c/busses/i2c-qup.c +4 -1

index 6227a5c..a91fc70 100644
@@ -192,6 +192,8 @@ struct qup_i2c_dev {
 	bool			is_dma;
 	/* To check if the current transfer is using DMA */
 	bool			use_dma;
+	/* The threshold length above which DMA will be used */
+	unsigned int		dma_threshold;
 	struct			dma_pool *dpool;
 	struct			qup_i2c_tag start_tag;
 	struct			qup_i2c_bam brx;

@@ -1294,7 +1296,8 @@ static int qup_i2c_xfer_v2(struct i2c_adapter 
 *adap,
 			   int num)
 {
 	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
-	int ret, len, idx = 0;
+	int ret, idx = 0;
+	unsigned int total_len = 0;
 
 	qup->bus_err = 0;

drivers/i2c/busses/i2c-qup.c +4 -5

 	qup->qup_err = 0;

@@ -1320,14 +1323,13 @@ static int qup_i2c_xfer_v2(struct i2c_adapter 
 *adap,
 				goto out;
 			}
 
-			len = (msgs[idx].len > qup->out_fifo_sz) ||
-			      (msgs[idx].len > qup->in_fifo_sz);
-
-			if (is_vmalloc_addr(msgs[idx].buf) || !len)
+			if (is_vmalloc_addr(msgs[idx].buf))
 				break;
+
+			total_len += msgs[idx].len;
 		}
 
-		if (idx == num)
+		if (idx == num && total_len > qup->dma_threshold)
 			qup->use_dma = true;
 	}

@@ -1587,6 +1589,7 @@ static int qup_i2c_probe(struct platform_device 
 *pdev)
 

drivers/i2c/busses/i2c-qup.c static int qup_i2c_probe(struct platform_device *pdev) · line 1592

Quoted 3 lines

drivers/i2c/busses/i2c-qup.c +1 -0

 	size = QUP_INPUT_FIFO_SIZE(io_mode);
 	qup->in_fifo_sz = qup->in_blk_sz * (2 << size);
+	qup->dma_threshold = min(qup->out_fifo_sz, qup->in_fifo_sz);

The patch is fine. small nit, you can avoid this global
dma_threshold. Instead
have it locally in qup_i2c_xfer_v2 itself.

Currently each message length in complete transfer is being checked for determining DMA mode and if any of the message length is less than FIFO length then non DMA mode is being used which will increase overhead. DMA can be used for any length and it should be determined with complete transfer length. Now, this patch introduces DMA threshold length and the transfer will be done in DMA mode if the total length is greater than this threshold length.

Signed-off-by Abhishek Sahu <[email protected]>

---
  drivers/i2c/busses/i2c-qup.c | 15 +++++++++------
  1 file changed, 9 insertions(+), 6 deletions(-)

drivers/i2c/busses/i2c-qup.c +9 -6

index 6227a5c..a91fc70 100644
@@ -192,6 +192,8 @@ struct qup_i2c_dev {
  	bool			is_dma;
  	/* To check if the current transfer is using DMA */
  	bool			use_dma;
+	/* The threshold length above which DMA will be used */
+	unsigned int		dma_threshold;
  	struct			dma_pool *dpool;
  	struct			qup_i2c_tag start_tag;
  	struct			qup_i2c_bam brx;

@@ -1294,7 +1296,8 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
  			   int num)
  {
  	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
-	int ret, len, idx = 0;
+	int ret, idx = 0;
+	unsigned int total_len = 0;
 
  	qup->bus_err = 0;
  	qup->qup_err = 0;

@@ -1320,14 +1323,13 @@ static int qup_i2c_xfer_v2(struct i2c_adapter *adap,
  				goto out;
  			}
 
-			len = (msgs[idx].len > qup->out_fifo_sz) ||
-			      (msgs[idx].len > qup->in_fifo_sz);
-
-			if (is_vmalloc_addr(msgs[idx].buf) || !len)
+			if (is_vmalloc_addr(msgs[idx].buf))
  				break;
+
+			total_len += msgs[idx].len;
  		}
 
-		if (idx == num)
+		if (idx == num && total_len > qup->dma_threshold)
  			qup->use_dma = true;
  	}
 

@@ -1587,6 +1589,7 @@ static int qup_i2c_probe(struct platform_device *pdev)
 
  	size = QUP_INPUT_FIFO_SIZE(io_mode);
  	qup->in_fifo_sz = qup->in_blk_sz * (2 << size);
+	qup->dma_threshold = min(qup->out_fifo_sz, qup->in_fifo_sz);
 
  	fs_div = ((src_clk_freq / clk_freq) / 2) - 3;
  	hs_div = 3;

Currently each message length in complete transfer is being checked for determining DMA mode and if any of the message length is less than FIFO length then non DMA mode is being used which will increase overhead. DMA can be used for any length and it should be determined with complete transfer length. Now, this patch introduces DMA threshold length and the transfer will be done in DMA mode if the total length is greater than this threshold length.

Signed-off-by Abhishek Sahu <[email protected]>

---

Currently the completion timeout is being taken according to maximum transfer length which is too high if SCL is operating in high frequency. This patch calculates timeout on the basis of one-byte transfer time and uses the same for completion timeout.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-qup.c +0 -0

index a91fc70..6df65ea 100644

drivers/i2c/busses/i2c-qup.c +4 -1

 /* Default values. Use these if FW query fails */
 #define DEFAULT_CLK_FREQ 100000

@@ -172,6 +172,7 @@ struct qup_i2c_dev {
 	int			in_blk_sz;
 
 	unsigned long		one_byte_t;
+	unsigned long		xfer_timeout;
 	struct qup_i2c_block	blk;
 
 	struct i2c_msg		*msg;

@@ -845,7 +846,7 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 		dma_async_issue_pending(qup->brx.dma);
 	}
 
-	if (!wait_for_completion_timeout(&qup->xfer, TOUT_MAX * HZ)) {
+	if (!wait_for_completion_timeout(&qup->xfer, qup->xfer_timeout)) {
 		dev_err(qup->dev, "normal trans timed out\n");
 		ret = -ETIMEDOUT;
 	}

@@ -1601,6 +1602,8 @@ static int qup_i2c_probe(struct platform_device *pdev)
 	 */
 	one_bit_t = (USEC_PER_SEC / clk_freq) + 1;
 	qup->one_byte_t = one_bit_t * 9;
+	qup->xfer_timeout = TOUT_MIN * HZ +
+			    usecs_to_jiffies(MX_TX_RX_LEN * qup->one_byte_t);
 
 	dev_dbg(qup->dev, "IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
 		qup->in_blk_sz, qup->in_fifo_sz,

Quoted 18 lines · Abhishek Sahu

Currently the completion timeout is being taken according to maximum transfer length which is too high if SCL is operating in high frequency. This patch calculates timeout on the basis of one-byte transfer time and uses the same for completion timeout.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-qup.c +0 -0

index a91fc70..6df65ea 100644
@@ -130,8 +130,8 @@
 #define MX_TX_RX_LEN			SZ_64K
 #define MX_BLOCKS			(MX_TX_RX_LEN / QUP_READ_LIMIT)
 

drivers/i2c/busses/i2c-qup.c line 135

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c +2 -2

-/* Max timeout in ms for 32k bytes */
-#define TOUT_MAX			300
+/* Min timeout for i2c transfers */
+#define TOUT_MIN			2
 

may be you can mention, why is this 2 ?

Quoted 19 lines · Sricharan R

Quoted 18 lines · Abhishek Sahu

Currently the completion timeout is being taken according to maximum transfer length which is too high if SCL is operating in high frequency. This patch calculates timeout on the basis of one-byte transfer time and uses the same for completion timeout.

Signed-off-by Abhishek Sahu <[email protected]>

---
drivers/i2c/busses/i2c-qup.c | 9 ++++++---

1 file changed, 6 insertions(+), 3 deletions(-)

drivers/i2c/busses/i2c-qup.c +0 -0

index a91fc70..6df65ea 100644
@@ -130,8 +130,8 @@
 #define MX_TX_RX_LEN			SZ_64K
 #define MX_BLOCKS			(MX_TX_RX_LEN / QUP_READ_LIMIT)
 

Quoted 8 lines

drivers/i2c/busses/i2c-qup.c line 135

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c +2 -2

-/* Max timeout in ms for 32k bytes */
-#define TOUT_MAX			300
+/* Min timeout for i2c transfers */
+#define TOUT_MIN			2
 

may be you can mention, why is this 2 ?

This 2 seconds is timeout which I am adding on the top of maximum xfer time calculated from bus speed to compensate the interrupt latency and other factors. It will make xfer timeout minimum as 2 seconds.

I will update the comment to explain it in more detail.

The BAM mode requires buffer for start tag data and tx, rx SG list. Currently, this is being taken for maximum transfer length (65K). But an I2C transfer can have multiple messages and each message can be of this maximum length so the buffer overflow will happen in this case. Since increasing buffer length won’t be feasible since an I2C transfer can contain any number of messages so this patch does following changes to make i2c transfers working for multiple messages case.

1. Calculate the required buffers for 2 maximum length messages (65K * 2).
2. Split the descriptor formation and descriptor scheduling. The idea is to fit as many messages in one DMA transfers for 65K threshold value (max_xfer_sg_len). Whenever the sg_cnt is crossing this, then schedule the BAM transfer and subsequent transfer will again start from zero.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 199 +++++++++++++++++++++++++------------------
 1 file changed, 118 insertions(+), 81 deletions(-)

drivers/i2c/busses/i2c-qup.c +106 -77

index 6df65ea..ba717bb 100644
@@ -155,6 +155,7 @@ struct qup_i2c_bam {
 	struct	qup_i2c_tag tag;
 	struct	dma_chan *dma;
 	struct	scatterlist *sg;
+	unsigned int sg_cnt;
 };
 
 struct qup_i2c_dev {

@@ -195,6 +196,8 @@ struct qup_i2c_dev {
 	bool			use_dma;
 	/* The threshold length above which DMA will be used */
 	unsigned int		dma_threshold;
+	unsigned int		max_xfer_sg_len;
+	unsigned int		tag_buf_pos;
 	struct			dma_pool *dpool;
 	struct			qup_i2c_tag start_tag;
 	struct			qup_i2c_bam brx;

@@ -699,86 +702,86 @@ static int qup_i2c_req_dma(struct qup_i2c_dev *qup)
 	return 0;
 }
 
-static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
-			       int num)
+static int qup_i2c_bam_make_desc(struct qup_i2c_dev *qup, struct i2c_msg *msg)
+{
+	int ret = 0, limit = QUP_READ_LIMIT;
+	u32 len = 0, blocks, rem;
+	u32 i = 0, tlen, tx_len = 0;
+	u8 *tags;
+
+	qup_i2c_set_blk_data(qup, msg);
+
+	blocks = qup->blk.count;
+	rem = msg->len - (blocks - 1) * limit;
+
+	if (msg->flags & I2C_M_RD) {
+		while (qup->blk.pos < blocks) {
+			tlen = (i == (blocks - 1)) ? rem : limit;
+			tags = &qup->start_tag.start[qup->tag_buf_pos + len];
+			len += qup_i2c_set_tags(tags, qup, msg);
+			qup->blk.data_len -= tlen;
+
+			/* scratch buf to read the start and len tags */
+			ret = qup_sg_set_buf(&qup->brx.sg[qup->brx.sg_cnt++],
+					     &qup->brx.tag.start[0],
+					     2, qup, DMA_FROM_DEVICE);
+
+			if (ret)
+				return ret;
+
+			ret = qup_sg_set_buf(&qup->brx.sg[qup->brx.sg_cnt++],
+					     &msg->buf[limit * i],
+					     tlen, qup,
+					     DMA_FROM_DEVICE);
+			if (ret)
+				return ret;
+
+			i++;
+			qup->blk.pos = i;
+		}
+		ret = qup_sg_set_buf(&qup->btx.sg[qup->btx.sg_cnt++],
+				     &qup->start_tag.start[qup->tag_buf_pos],
+				     len, qup, DMA_TO_DEVICE);
+		if (ret)
+			return ret;
+
+		qup->tag_buf_pos += len;
+	} else {
+		while (qup->blk.pos < blocks) {
+			tlen = (i == (blocks - 1)) ? rem : limit;
+			tags = &qup->start_tag.start[qup->tag_buf_pos + tx_len];
+			len = qup_i2c_set_tags(tags, qup, msg);
+			qup->blk.data_len -= tlen;
+
+			ret = qup_sg_set_buf(&qup->btx.sg[qup->btx.sg_cnt++],
+					     tags, len,
+					     qup, DMA_TO_DEVICE);
+			if (ret)
+				return ret;
+
+			tx_len += len;
+			ret = qup_sg_set_buf(&qup->btx.sg[qup->btx.sg_cnt++],
+					     &msg->buf[limit * i],
+					     tlen, qup, DMA_TO_DEVICE);
+			if (ret)
+				return ret;
+			i++;
+			qup->blk.pos = i;
+		}
+
+		qup->tag_buf_pos += tx_len;
+	}
+
+	return 0;
+}
+
+static int qup_i2c_bam_schedule_desc(struct qup_i2c_dev *qup)
 {
 	struct dma_async_tx_descriptor *txd, *rxd = NULL;
-	int ret = 0, idx = 0, limit = QUP_READ_LIMIT;
+	int ret = 0;
 	dma_cookie_t cookie_rx, cookie_tx;
-	u32 len, blocks, rem;
-	u32 i, tlen, tx_len, tx_buf = 0, rx_buf = 0, off = 0;
-	u8 *tags;
-
-	while (idx < num) {
-		tx_len = 0, len = 0, i = 0;
-
-		qup->is_last = (idx == (num - 1));
-
-		qup_i2c_set_blk_data(qup, msg);
-
-		blocks = qup->blk.count;
-		rem = msg->len - (blocks - 1) * limit;
-
-		if (msg->flags & I2C_M_RD) {
-			while (qup->blk.pos < blocks) {
-				tlen = (i == (blocks - 1)) ? rem : limit;
-				tags = &qup->start_tag.start[off + len];
-				len += qup_i2c_set_tags(tags, qup, msg);
-				qup->blk.data_len -= tlen;
-
-				/* scratch buf to read the start and len tags */
-				ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
-						     &qup->brx.tag.start[0],
-						     2, qup, DMA_FROM_DEVICE);
-
-				if (ret)
-					return ret;
-
-				ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
-						     &msg->buf[limit * i],
-						     tlen, qup,
-						     DMA_FROM_DEVICE);
-				if (ret)
-					return ret;
-
-				i++;
-				qup->blk.pos = i;
-			}
-			ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
-					     &qup->start_tag.start[off],
-					     len, qup, DMA_TO_DEVICE);
-			if (ret)
-				return ret;
-
-			off += len;
-		} else {
-			while (qup->blk.pos < blocks) {
-				tlen = (i == (blocks - 1)) ? rem : limit;
-				tags = &qup->start_tag.start[off + tx_len];
-				len = qup_i2c_set_tags(tags, qup, msg);
-				qup->blk.data_len -= tlen;
-
-				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
-						     tags, len,
-						     qup, DMA_TO_DEVICE);
-				if (ret)
-					return ret;
-
-				tx_len += len;
-				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
-						     &msg->buf[limit * i],
-						     tlen, qup, DMA_TO_DEVICE);
-				if (ret)
-					return ret;
-				i++;
-				qup->blk.pos = i;
-			}
-			off += tx_len;
-
-		}
-		idx++;
-		msg++;
-	}
+	u32 len = 0;
+	u32 tx_buf = qup->btx.sg_cnt, rx_buf = qup->brx.sg_cnt;
 
 	/* schedule the EOT and FLUSH I2C tags */
 	len = 1;

@@ -878,11 +881,19 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
 	return ret;
 }
 
+static void qup_i2c_bam_clear_tag_buffers(struct qup_i2c_dev *qup)
+{
+	qup->btx.sg_cnt = 0;
+	qup->brx.sg_cnt = 0;
+	qup->tag_buf_pos = 0;
+}
+
 static int qup_i2c_bam_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
 			    int num)
 {
 	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
 	int ret = 0;
+	int idx = 0;
 
 	enable_irq(qup->irq);
 	ret = qup_i2c_req_dma(qup);

@@ -905,9 +916,34 @@ static int qup_i2c_bam_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
 		goto out;
 
 	writel(qup->clk_ctl, qup->base + QUP_I2C_CLK_CTL);
+	qup_i2c_bam_clear_tag_buffers(qup);
+
+	for (idx = 0; idx < num; idx++) {
+		qup->msg = msg + idx;
+		qup->is_last = idx == (num - 1);
+
+		ret = qup_i2c_bam_make_desc(qup, qup->msg);
+		if (ret)
+			break;
+
+		/*
+		 * Make DMA descriptor and schedule the BAM transfer if its
+		 * already crossed the maximum length. Since the memory for all
+		 * tags buffers have been taken for 2 maximum possible
+		 * transfers length so it will never cross the buffer actual
+		 * length.
+		 */
+		if (qup->btx.sg_cnt > qup->max_xfer_sg_len ||

drivers/i2c/busses/i2c-qup.c +3 -4

-	qup->msg = msg;
-	ret = qup_i2c_bam_do_xfer(qup, qup->msg, num);
 out:
 	disable_irq(qup->irq);
 

@@ -1459,7 +1495,8 @@ static int qup_i2c_probe(struct platform_device *pdev)
 		else if (ret != 0)
 			goto nodma;
 
-		blocks = (MX_BLOCKS << 1) + 1;
+		qup->max_xfer_sg_len = (MX_BLOCKS << 1);
+		blocks = 2 * qup->max_xfer_sg_len + 1;
 		qup->btx.sg = devm_kzalloc(&pdev->dev,
 					   sizeof(*qup->btx.sg) * blocks,
 					   GFP_KERNEL);

@@ -1603,7 +1640,7 @@ static int qup_i2c_probe(struct platform_device *pdev)
 	one_bit_t = (USEC_PER_SEC / clk_freq) + 1;
 	qup->one_byte_t = one_bit_t * 9;
 	qup->xfer_timeout = TOUT_MIN * HZ +
-			    usecs_to_jiffies(MX_TX_RX_LEN * qup->one_byte_t);
+		usecs_to_jiffies(2 * MX_TX_RX_LEN * qup->one_byte_t);
 
 	dev_dbg(qup->dev, "IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
 		qup->in_blk_sz, qup->in_fifo_sz,

Hi Abhishek,

Quoted 16 lines · Abhishek Sahu

The BAM mode requires buffer for start tag data and tx, rx SG list. Currently, this is being taken for maximum transfer length (65K). But an I2C transfer can have multiple messages and each message can be of this maximum length so the buffer overflow will happen in this case. Since increasing buffer length won’t be feasible since an I2C transfer can contain any number of messages so this patch does following changes to make i2c transfers working for multiple messages case.

1. Calculate the required buffers for 2 maximum length messages (65K * 2).
2. Split the descriptor formation and descriptor scheduling. The idea is to fit as many messages in one DMA transfers for 65K threshold value (max_xfer_sg_len). Whenever the sg_cnt is crossing this, then schedule the BAM transfer and subsequent transfer will again start from zero.

Quoted 46 lines

drivers/i2c/busses/i2c-qup.c +7 -0

+static void qup_i2c_bam_clear_tag_buffers(struct qup_i2c_dev *qup)
+{
+	qup->btx.sg_cnt = 0;
+	qup->brx.sg_cnt = 0;
+	qup->tag_buf_pos = 0;
+}
+
 static int qup_i2c_bam_xfer(struct i2c_adapter *adap, struct i2c_msg 

*msg,

drivers/i2c/busses/i2c-qup.c +25 -0

 			    int num)
 {
 	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
 	int ret = 0;
+	int idx = 0;
 
 	enable_irq(qup->irq);
 	ret = qup_i2c_req_dma(qup);

@@ -905,9 +916,34 @@ static int qup_i2c_bam_xfer(struct i2c_adapter 
 *adap, struct i2c_msg *msg,
 		goto out;
 
 	writel(qup->clk_ctl, qup->base + QUP_I2C_CLK_CTL);
+	qup_i2c_bam_clear_tag_buffers(qup);
+
+	for (idx = 0; idx < num; idx++) {
+		qup->msg = msg + idx;
+		qup->is_last = idx == (num - 1);
+
+		ret = qup_i2c_bam_make_desc(qup, qup->msg);
+		if (ret)
+			break;
+
+		/*
+		 * Make DMA descriptor and schedule the BAM transfer if its
+		 * already crossed the maximum length. Since the memory for all
+		 * tags buffers have been taken for 2 maximum possible
+		 * transfers length so it will never cross the buffer actual
+		 * length.
+		 */
+		if (qup->btx.sg_cnt > qup->max_xfer_sg_len ||
+		    qup->brx.sg_cnt > qup->max_xfer_sg_len ||
+		    qup->is_last) {
+			ret = qup_i2c_bam_schedule_desc(qup);
+			if (ret)
+				break;
+

drivers/i2c/busses/i2c-qup.c static int qup_i2c_bam_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, · line 946

Quoted 4 lines

drivers/i2c/busses/i2c-qup.c +3 -0

+			qup_i2c_bam_clear_tag_buffers(qup);
+		}
+	}
 

hmm, is this because of only stress tests or was there any device
which
was using i2c for multiple messages exceeding 64k bytes ?

Infact we are trying to club two separate messages together across
64k
boundaries. Not sure if its really correct. So either we club all
messages
fully or club only up to the length that would cover the whole
message < 64K
and send the remaining whole messages in next transfer.

The BAM mode requires buffer for start tag data and tx, rx SG list. Currently, this is being taken for maximum transfer length (65K). But an I2C transfer can have multiple messages and each message can be of this maximum length so the buffer overflow will happen in this case. Since increasing buffer length won’t be feasible since an I2C transfer can contain any number of messages so this patch does following changes to make i2c transfers working for multiple messages case.

1. Calculate the required buffers for 2 maximum length messages

    (65K * 2).

2. Split the descriptor formation and descriptor scheduling. The idea is to fit as many messages in one DMA transfers for 65K threshold value (max_xfer_sg_len). Whenever the sg_cnt is crossing this, then schedule the BAM transfer and subsequent transfer will again start from zero.

Signed-off-by Abhishek Sahu <[email protected]>

---
  drivers/i2c/busses/i2c-qup.c | 199 +++++++++++++++++++++++++------------------
  1 file changed, 118 insertions(+), 81 deletions(-)

drivers/i2c/busses/i2c-qup.c +117 -80

index 6df65ea..ba717bb 100644
@@ -155,6 +155,7 @@ struct qup_i2c_bam {
  	struct	qup_i2c_tag tag;
  	struct	dma_chan *dma;
  	struct	scatterlist *sg;
+	unsigned int sg_cnt;
  };
 
  struct qup_i2c_dev {

@@ -195,6 +196,8 @@ struct qup_i2c_dev {
  	bool			use_dma;
  	/* The threshold length above which DMA will be used */
  	unsigned int		dma_threshold;
+	unsigned int		max_xfer_sg_len;
+	unsigned int		tag_buf_pos;
  	struct			dma_pool *dpool;
  	struct			qup_i2c_tag start_tag;
  	struct			qup_i2c_bam brx;

@@ -699,86 +702,86 @@ static int qup_i2c_req_dma(struct qup_i2c_dev *qup)
  	return 0;
  }
 
-static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
-			       int num)
+static int qup_i2c_bam_make_desc(struct qup_i2c_dev *qup, struct i2c_msg *msg)
+{
+	int ret = 0, limit = QUP_READ_LIMIT;
+	u32 len = 0, blocks, rem;
+	u32 i = 0, tlen, tx_len = 0;
+	u8 *tags;
+
+	qup_i2c_set_blk_data(qup, msg);
+
+	blocks = qup->blk.count;
+	rem = msg->len - (blocks - 1) * limit;
+
+	if (msg->flags & I2C_M_RD) {
+		while (qup->blk.pos < blocks) {
+			tlen = (i == (blocks - 1)) ? rem : limit;
+			tags = &qup->start_tag.start[qup->tag_buf_pos + len];
+			len += qup_i2c_set_tags(tags, qup, msg);
+			qup->blk.data_len -= tlen;
+
+			/* scratch buf to read the start and len tags */
+			ret = qup_sg_set_buf(&qup->brx.sg[qup->brx.sg_cnt++],
+					     &qup->brx.tag.start[0],
+					     2, qup, DMA_FROM_DEVICE);
+
+			if (ret)
+				return ret;
+
+			ret = qup_sg_set_buf(&qup->brx.sg[qup->brx.sg_cnt++],
+					     &msg->buf[limit * i],
+					     tlen, qup,
+					     DMA_FROM_DEVICE);
+			if (ret)
+				return ret;
+
+			i++;
+			qup->blk.pos = i;
+		}
+		ret = qup_sg_set_buf(&qup->btx.sg[qup->btx.sg_cnt++],
+				     &qup->start_tag.start[qup->tag_buf_pos],
+				     len, qup, DMA_TO_DEVICE);
+		if (ret)
+			return ret;
+
+		qup->tag_buf_pos += len;
+	} else {
+		while (qup->blk.pos < blocks) {
+			tlen = (i == (blocks - 1)) ? rem : limit;
+			tags = &qup->start_tag.start[qup->tag_buf_pos + tx_len];
+			len = qup_i2c_set_tags(tags, qup, msg);
+			qup->blk.data_len -= tlen;
+
+			ret = qup_sg_set_buf(&qup->btx.sg[qup->btx.sg_cnt++],
+					     tags, len,
+					     qup, DMA_TO_DEVICE);
+			if (ret)
+				return ret;
+
+			tx_len += len;
+			ret = qup_sg_set_buf(&qup->btx.sg[qup->btx.sg_cnt++],
+					     &msg->buf[limit * i],
+					     tlen, qup, DMA_TO_DEVICE);
+			if (ret)
+				return ret;
+			i++;
+			qup->blk.pos = i;
+		}
+
+		qup->tag_buf_pos += tx_len;
+	}
+
+	return 0;
+}
+
+static int qup_i2c_bam_schedule_desc(struct qup_i2c_dev *qup)
  {
  	struct dma_async_tx_descriptor *txd, *rxd = NULL;
-	int ret = 0, idx = 0, limit = QUP_READ_LIMIT;
+	int ret = 0;
  	dma_cookie_t cookie_rx, cookie_tx;
-	u32 len, blocks, rem;
-	u32 i, tlen, tx_len, tx_buf = 0, rx_buf = 0, off = 0;
-	u8 *tags;
-
-	while (idx < num) {
-		tx_len = 0, len = 0, i = 0;
-
-		qup->is_last = (idx == (num - 1));
-
-		qup_i2c_set_blk_data(qup, msg);
-
-		blocks = qup->blk.count;
-		rem = msg->len - (blocks - 1) * limit;
-
-		if (msg->flags & I2C_M_RD) {
-			while (qup->blk.pos < blocks) {
-				tlen = (i == (blocks - 1)) ? rem : limit;
-				tags = &qup->start_tag.start[off + len];
-				len += qup_i2c_set_tags(tags, qup, msg);
-				qup->blk.data_len -= tlen;
-
-				/* scratch buf to read the start and len tags */
-				ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
-						     &qup->brx.tag.start[0],
-						     2, qup, DMA_FROM_DEVICE);
-
-				if (ret)
-					return ret;
-
-				ret = qup_sg_set_buf(&qup->brx.sg[rx_buf++],
-						     &msg->buf[limit * i],
-						     tlen, qup,
-						     DMA_FROM_DEVICE);
-				if (ret)
-					return ret;
-
-				i++;
-				qup->blk.pos = i;
-			}
-			ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
-					     &qup->start_tag.start[off],
-					     len, qup, DMA_TO_DEVICE);
-			if (ret)
-				return ret;
-
-			off += len;
-		} else {
-			while (qup->blk.pos < blocks) {
-				tlen = (i == (blocks - 1)) ? rem : limit;
-				tags = &qup->start_tag.start[off + tx_len];
-				len = qup_i2c_set_tags(tags, qup, msg);
-				qup->blk.data_len -= tlen;
-
-				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
-						     tags, len,
-						     qup, DMA_TO_DEVICE);
-				if (ret)
-					return ret;
-
-				tx_len += len;
-				ret = qup_sg_set_buf(&qup->btx.sg[tx_buf++],
-						     &msg->buf[limit * i],
-						     tlen, qup, DMA_TO_DEVICE);
-				if (ret)
-					return ret;
-				i++;
-				qup->blk.pos = i;
-			}
-			off += tx_len;
-
-		}
-		idx++;
-		msg++;
-	}
+	u32 len = 0;
+	u32 tx_buf = qup->btx.sg_cnt, rx_buf = qup->brx.sg_cnt;
 
  	/* schedule the EOT and FLUSH I2C tags */
  	len = 1;

@@ -878,11 +881,19 @@ static int qup_i2c_bam_do_xfer(struct qup_i2c_dev *qup, struct i2c_msg *msg,
  	return ret;
  }
 
+static void qup_i2c_bam_clear_tag_buffers(struct qup_i2c_dev *qup)
+{
+	qup->btx.sg_cnt = 0;
+	qup->brx.sg_cnt = 0;
+	qup->tag_buf_pos = 0;
+}
+
  static int qup_i2c_bam_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
  			    int num)
  {
  	struct qup_i2c_dev *qup = i2c_get_adapdata(adap);
  	int ret = 0;
+	int idx = 0;
 
  	enable_irq(qup->irq);
  	ret = qup_i2c_req_dma(qup);

@@ -905,9 +916,34 @@ static int qup_i2c_bam_xfer(struct i2c_adapter *adap, struct i2c_msg *msg,
  		goto out;
 
  	writel(qup->clk_ctl, qup->base + QUP_I2C_CLK_CTL);
+	qup_i2c_bam_clear_tag_buffers(qup);
+
+	for (idx = 0; idx < num; idx++) {
+		qup->msg = msg + idx;
+		qup->is_last = idx == (num - 1);
+
+		ret = qup_i2c_bam_make_desc(qup, qup->msg);
+		if (ret)
+			break;
+
+		/*
+		 * Make DMA descriptor and schedule the BAM transfer if its
+		 * already crossed the maximum length. Since the memory for all
+		 * tags buffers have been taken for 2 maximum possible
+		 * transfers length so it will never cross the buffer actual
+		 * length.
+		 */
+		if (qup->btx.sg_cnt > qup->max_xfer_sg_len ||
+		    qup->brx.sg_cnt > qup->max_xfer_sg_len ||
+		    qup->is_last) {
+			ret = qup_i2c_bam_schedule_desc(qup);
+			if (ret)
+				break;
+
+			qup_i2c_bam_clear_tag_buffers(qup);
+		}
+	}
 
-	qup->msg = msg;
-	ret = qup_i2c_bam_do_xfer(qup, qup->msg, num);
  out:
  	disable_irq(qup->irq);
 

@@ -1459,7 +1495,8 @@ static int qup_i2c_probe(struct platform_device *pdev)
  		else if (ret != 0)
  			goto nodma;
 
-		blocks = (MX_BLOCKS << 1) + 1;
+		qup->max_xfer_sg_len = (MX_BLOCKS << 1);
+		blocks = 2 * qup->max_xfer_sg_len + 1;
  		qup->btx.sg = devm_kzalloc(&pdev->dev,
  					   sizeof(*qup->btx.sg) * blocks,
  					   GFP_KERNEL);

drivers/i2c/busses/i2c-qup.c +0 -0

  	dev_dbg(qup->dev, "IN:block:%d, fifo:%d, OUT:block:%d, fifo:%d\n",
  		qup->in_blk_sz, qup->in_fifo_sz,
 

Hey Abhishek,

Quoted 17 lines · Abhishek Sahu

The BAM mode requires buffer for start tag data and tx, rx SG list. Currently, this is being taken for maximum transfer length (65K). But an I2C transfer can have multiple messages and each message can be of this maximum length so the buffer overflow will happen in this case. Since increasing buffer length won’t be feasible since an I2C transfer can contain any number of messages so this patch does following changes to make i2c transfers working for multiple messages case.

1. Calculate the required buffers for 2 maximum length messages

    (65K * 2).

2. Split the descriptor formation and descriptor scheduling. The idea is to fit as many messages in one DMA transfers for 65K threshold value (max_xfer_sg_len). Whenever the sg_cnt is crossing this, then schedule the BAM transfer and subsequent transfer will again start from zero.

drivers/i2c/busses/i2c-qup.c static int qup_i2c_probe(struct platform_device · line 1644

Quoted 7 lines

drivers/i2c/busses/i2c-qup.c +1 -1

@@ -1603,7 +1640,7 @@ static int qup_i2c_probe(struct platform_device 
 *pdev)
  	one_bit_t = (USEC_PER_SEC / clk_freq) + 1;
  	qup->one_byte_t = one_bit_t * 9;
  	qup->xfer_timeout = TOUT_MIN * HZ +
-			    usecs_to_jiffies(MX_TX_RX_LEN * qup->one_byte_t);
+		usecs_to_jiffies(2 * MX_TX_RX_LEN * qup->one_byte_t);

Maybe it would make sense to add a comment here explaining why the magic number 2 has been added.

The BAM mode requires buffer for start tag data and tx, rx SG list. Currently, this is being taken for maximum transfer length (65K). But an I2C transfer can have multiple messages and each message can be of this maximum length so the buffer overflow will happen in this case. Since increasing buffer length won’t be feasible since an I2C transfer can contain any number of messages so this patch does following changes to make i2c transfers working for multiple messages case.

1. Calculate the required buffers for 2 maximum length messages (65K * 2).
2. Split the descriptor formation and descriptor scheduling. The idea is to fit as many messages in one DMA transfers for 65K threshold value (max_xfer_sg_len). Whenever the sg_cnt is crossing this, then schedule the BAM transfer and subsequent transfer will again start from zero.

Signed-off-by Abhishek Sahu <[email protected]>

Quoted 17 lines · Abhishek Sahu

The BAM mode requires buffer for start tag data and tx, rx SG list. Currently, this is being taken for maximum transfer length (65K). But an I2C transfer can have multiple messages and each message can be of this maximum length so the buffer overflow will happen in this case. Since increasing buffer length won’t be feasible since an I2C transfer can contain any number of messages so this patch does following changes to make i2c transfers working for multiple messages case.

1. Calculate the required buffers for 2 maximum length messages (65K * 2).
2. Split the descriptor formation and descriptor scheduling. The idea is to fit as many messages in one DMA transfers for 65K threshold value (max_xfer_sg_len). Whenever the sg_cnt is crossing this, then schedule the BAM transfer and subsequent transfer will again start from zero.

Quoted 1 line

Signed-off-by Abhishek Sahu <[email protected]>

I'm ok with this patch. I find the idea of a > 64k size message to be something that usually wouldn't be encountered, but... with some eeproms and maybe TPMs perhaps this could happen?

Reviewed-by Andy Gross <[email protected]>

According to I2c specification, “If a master-receiver sends a repeated START condition, it sends a not-acknowledge (A) just before the repeated START condition”. QUP v2 supports sending of NACK without stop with QUP_TAG_V2_DATARD_NACK so added the same.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-qup.c +1 -0

index ba717bb..edea3b9 100644
@@ -113,6 +113,7 @@
 #define QUP_TAG_V2_DATAWR              0x82
 #define QUP_TAG_V2_DATAWR_STOP         0x83
 #define QUP_TAG_V2_DATARD              0x85
+#define QUP_TAG_V2_DATARD_NACK         0x86
 #define QUP_TAG_V2_DATARD_STOP         0x87
 
 /* Status, Error flags */

According to I2c specification, “If a master-receiver sends a repeated START condition, it sends a not-acknowledge (A) just before the repeated START condition”. QUP v2 supports sending of NACK without stop with QUP_TAG_V2_DATARD_NACK so added the same.

Signed-off-by Abhishek Sahu <[email protected]>

---
  drivers/i2c/busses/i2c-qup.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)

drivers/i2c/busses/i2c-qup.c +4 -1

index ba717bb..edea3b9 100644
@@ -113,6 +113,7 @@
  #define QUP_TAG_V2_DATAWR              0x82
  #define QUP_TAG_V2_DATAWR_STOP         0x83
  #define QUP_TAG_V2_DATARD              0x85
+#define QUP_TAG_V2_DATARD_NACK         0x86
  #define QUP_TAG_V2_DATARD_STOP         0x87
 
  /* Status, Error flags */

@@ -609,7 +610,9 @@ static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
  			tags[len++] = QUP_TAG_V2_DATAWR_STOP;
  	} else {
  		if (msg->flags & I2C_M_RD)
-			tags[len++] = QUP_TAG_V2_DATARD;
+			tags[len++] = qup->blk.pos == (qup->blk.count - 1) ?
+				      QUP_TAG_V2_DATARD_NACK :
+				      QUP_TAG_V2_DATARD;
  		else
  			tags[len++] = QUP_TAG_V2_DATAWR;
  	}

According to I2c specification, “If a master-receiver sends a repeated START condition, it sends a not-acknowledge (A) just before the repeated START condition”. QUP v2 supports sending of NACK without stop with QUP_TAG_V2_DATARD_NACK so added the same.

Signed-off-by Abhishek Sahu <[email protected]>

Following are the major issues in current driver code

1. The current driver simply assumes the transfer completion whenever its gets any non-error interrupts and then simply do the polling of available/free bytes in FIFO.
2. The block mode is not working properly since no handling in being done for OUT_BLOCK_WRITE_REQ and IN_BLOCK_READ_REQ.

Because of above, i2c v1 transfers of size greater than 32 are failing with following error message

i2c_qup 78b6000.i2c: timeout for fifo out full

To make block mode working properly and move to use the interrupts instead of polling, major code reorganization is required. Following are the major changes done in this patch

1. Remove the polling of TX FIFO free space and RX FIFO available bytes and move to interrupts completely. QUP has QUP_MX_OUTPUT_DONE, QUP_MX_INPUT_DONE, OUT_BLOCK_WRITE_REQ and IN_BLOCK_READ_REQ interrupts to handle FIFO’s properly so check all these interrupts.
2. During write, For FIFO mode, TX FIFO can be directly written without checking for FIFO space. For block mode, the QUP will generate OUT_BLOCK_WRITE_REQ interrupt whenever it has block size of available space.
3. During read, both TX and RX FIFO will be used. TX will be used for writing tags and RX will be used for receiving the data. In QUP, TX and RX can operate in separate mode so configure modes accordingly.
4. For read FIFO mode, wait for QUP_MX_INPUT_DONE interrupt which will be generated after all the bytes have been copied in RX FIFO. For read Block mode, QUP will generate IN_BLOCK_READ_REQ interrupts whenever it has block size of available data.

Signed-off-by Abhishek Sahu <[email protected]>

---
 drivers/i2c/busses/i2c-qup.c | 399 ++++++++++++++++++++++++++++---------------
 1 file changed, 257 insertions(+), 142 deletions(-)

drivers/i2c/busses/i2c-qup.c +93 -9

index edea3b9..af6c21a 100644
@@ -73,8 +73,11 @@
 #define QUP_IN_SVC_FLAG		BIT(9)
 #define QUP_MX_OUTPUT_DONE	BIT(10)
 #define QUP_MX_INPUT_DONE	BIT(11)
+#define OUT_BLOCK_WRITE_REQ	BIT(12)
+#define IN_BLOCK_READ_REQ	BIT(13)
 
 /* I2C mini core related values */
+#define QUP_NO_INPUT		BIT(7)
 #define QUP_CLOCK_AUTO_GATE	BIT(13)
 #define I2C_MINI_CORE		(2 << 8)
 #define I2C_N_VAL		15

@@ -138,13 +141,51 @@
 #define DEFAULT_CLK_FREQ 100000
 #define DEFAULT_SRC_CLK 20000000
 
+/* MAX_OUTPUT_DONE_FLAG has been received */
+#define QUP_BLK_EVENT_TX_IRQ_DONE		BIT(0)
+/* MAX_INPUT_DONE_FLAG has been received */
+#define QUP_BLK_EVENT_RX_IRQ_DONE		BIT(1)
+/* All the TX bytes have been written in TX FIFO */
+#define QUP_BLK_EVENT_TX_DATA_DONE		BIT(2)
+/* All the RX bytes have been read from RX FIFO */
+#define QUP_BLK_EVENT_RX_DATA_DONE		BIT(3)
+
+/* All the required events to mark a QUP I2C TX transfer completed */
+#define QUP_BLK_EVENT_TX_DONE		(QUP_BLK_EVENT_TX_IRQ_DONE | \
+					 QUP_BLK_EVENT_TX_DATA_DONE)
+/* All the required events to mark a QUP I2C RX transfer completed */
+#define QUP_BLK_EVENT_RX_DONE		(QUP_BLK_EVENT_TX_DONE | \
+					 QUP_BLK_EVENT_RX_IRQ_DONE | \
+					 QUP_BLK_EVENT_RX_DATA_DONE)
+
+/*
+ * count: no of blocks
+ * pos: current block number
+ * tx_tag_len: tx tag length for current block
+ * rx_tag_len: rx tag length for current block
+ * data_len: remaining data length for current message
+ * total_tx_len: total tx length including tag bytes for current QUP transfer
+ * total_rx_len: total rx length including tag bytes for current QUP transfer
+ * tx_fifo_free: number of free bytes in current QUP block write.
+ * fifo_available: number of available bytes in RX FIFO for current
+ *		   QUP block read
+ * is_tx_blk_mode: whether tx uses block or FIFO mode in case of non BAM xfer.
+ * is_rx_blk_mode: whether rx uses block or FIFO mode in case of non BAM xfer.
+ * tags: contains tx tag bytes for current QUP transfer
+ */
 struct qup_i2c_block {
-	int	count;
-	int	pos;
-	int	tx_tag_len;
-	int	rx_tag_len;
-	int	data_len;
-	u8	tags[6];
+	int		count;
+	int		pos;
+	int		tx_tag_len;
+	int		rx_tag_len;
+	int		data_len;
+	int		total_tx_len;
+	int		total_rx_len;
+	int		tx_fifo_free;
+	int		fifo_available;
+	bool		is_tx_blk_mode;
+	bool		is_rx_blk_mode;
+	u8		tags[6];
 };
 
 struct qup_i2c_tag {

@@ -187,6 +228,7 @@ struct qup_i2c_dev {
 
 	/* To check if this is the last msg */
 	bool			is_last;
+	bool			is_qup_v1;
 
 	/* To configure when bus is in run state */
 	int			config_run;

@@ -195,6 +237,10 @@ struct qup_i2c_dev {
 	bool			is_dma;
 	/* To check if the current transfer is using DMA */
 	bool			use_dma;
+	/* Required events to mark QUP transfer as completed */
+	u32			blk_events;
+	/* Already completed events in QUP transfer */
+	u32			cur_blk_events;
 	/* The threshold length above which DMA will be used */
 	unsigned int		dma_threshold;
 	unsigned int		max_xfer_sg_len;

@@ -205,11 +251,18 @@ struct qup_i2c_dev {
 	struct			qup_i2c_bam btx;
 
 	struct completion	xfer;
+	/* function to write data in tx fifo */
+	void (*write_tx_fifo)(struct qup_i2c_dev *qup);
+	/* function to read data from rx fifo */
+	void (*read_rx_fifo)(struct qup_i2c_dev *qup);
+	/* function to write tags in tx fifo for i2c read transfer */
+	void (*write_rx_tags)(struct qup_i2c_dev *qup);
 };
 
 static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
 {
 	struct qup_i2c_dev *qup = dev;
+	struct qup_i2c_block *blk = &qup->blk;
 	u32 bus_err;
 	u32 qup_err;
 	u32 opflags;

@@ -256,12 +309,54 @@ static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
 		goto done;
 	}
 
-	if (opflags & QUP_IN_SVC_FLAG)
-		writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL);
+	if (!qup->is_qup_v1)
+		goto done;
 
-	if (opflags & QUP_OUT_SVC_FLAG)
+	if (opflags & QUP_OUT_SVC_FLAG) {
 		writel(QUP_OUT_SVC_FLAG, qup->base + QUP_OPERATIONAL);
 
+		/*
+		 * Ideally, would like to check QUP_MAX_OUTPUT_DONE_FLAG.
+		 * However, QUP_MAX_OUTPUT_DONE_FLAG is lagging behind
+		 * QUP_OUTPUT_SERVICE_FLAG. The only reason for
+		 * QUP_OUTPUT_SERVICE_FLAG to be set in FIFO mode is
+		 * QUP_MAX_OUTPUT_DONE_FLAG condition. The code checking
+		 * here QUP_OUTPUT_SERVICE_FLAG and assumes that
+		 * QUP_MAX_OUTPUT_DONE_FLAG.
+		 */
+		if (!blk->is_tx_blk_mode)
+			qup->cur_blk_events |= QUP_BLK_EVENT_TX_IRQ_DONE;
+
+		if (opflags & OUT_BLOCK_WRITE_REQ) {
+			blk->tx_fifo_free += qup->out_blk_sz;
+			if (qup->msg->flags & I2C_M_RD)
+				qup->write_rx_tags(qup);
+			else
+				qup->write_tx_fifo(qup);
+		}
+	}
+
+	if (opflags & QUP_IN_SVC_FLAG) {
+		writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL);
+
+		if (!blk->is_rx_blk_mode) {
+			blk->fifo_available += qup->in_fifo_sz;
+			qup->read_rx_fifo(qup);
+		} else if (opflags & IN_BLOCK_READ_REQ) {
+			blk->fifo_available += qup->in_blk_sz;
+			qup->read_rx_fifo(qup);
+		}