The I2C driver gets an interrupt upon transfer completion.
For multiple messages in a single transfer, N interrupts will be
received for N messages, leading to significant software interrupt
latency. To mitigate this latency, utilize Block Event Interrupt (BEI)
only when an interrupt is necessary. This means large transfers can be
split into multiple chunks of 64 messages internally, without expecting
interrupts for the first 63 transfers, only the last one will trigger
an interrupt indicating 64 transfers completed.

By implementing BEI, multi-message transfers can be divided into
chunks of 64 messages, improving overall transfer time.
This optimization reduces transfer time from 168 ms to 48 ms for a
series of 200 I2C write messages in a single transfer, with a
clock frequency support of 100 kHz.

BEI optimizations are currently implemented for I2C write transfers only,
as there is no use case for multiple I2C read messages in a single transfer
at this time.

Signed-off-by: Jyothi Kumar Seerapu <quic_jseerapu@quicinc.com>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 205 +++++++++++++++++++++++++----
 1 file changed, 181 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 212336f724a6..a73dc1738a62 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -99,6 +99,10 @@ struct geni_i2c_dev {
 	struct dma_chan *rx_c;
 	bool gpi_mode;
 	bool abort_done;
+	bool is_tx_multi_xfer;
+	u32 num_msgs;
+	u32 tx_irq_cnt;
+	struct gpi_i2c_config *gpi_config;
 };
 
 struct geni_i2c_desc {
@@ -485,6 +489,7 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 static void i2c_gpi_cb_result(void *cb, const struct dmaengine_result *result)
 {
 	struct geni_i2c_dev *gi2c = cb;
+	struct gpi_multi_xfer *tx_multi_xfer;
 
 	if (result->result != DMA_TRANS_NOERROR) {
 		dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
@@ -493,7 +498,21 @@ static void i2c_gpi_cb_result(void *cb, const struct dmaengine_result *result)
 		dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue);
 	}
 
-	complete(&gi2c->done);
+	if (gi2c->is_tx_multi_xfer) {
+		tx_multi_xfer = &gi2c->gpi_config->multi_xfer;
+
+		/*
+		 * Send Completion for last message or multiple of NUM_MSGS_PER_IRQ.
+		 */
+		if ((tx_multi_xfer->irq_msg_cnt == gi2c->num_msgs - 1) ||
+		    (!((tx_multi_xfer->irq_msg_cnt + 1) % NUM_MSGS_PER_IRQ))) {
+			tx_multi_xfer->irq_cnt++;
+			complete(&gi2c->done);
+		}
+		tx_multi_xfer->irq_msg_cnt++;
+	} else {
+		complete(&gi2c->done);
+	}
 }
 
 static void geni_i2c_gpi_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
@@ -511,7 +530,41 @@ static void geni_i2c_gpi_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	}
 }
 
-static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
+/**
+ * gpi_i2c_multi_desc_unmap() - unmaps the buffers post multi message TX transfers
+ * @dev: pointer to the corresponding dev node
+ * @gi2c: i2c dev handle
+ * @msgs: i2c messages array
+ * @peripheral: pointer to the gpi_i2c_config
+ */
+static void gpi_i2c_multi_desc_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
+				     struct gpi_i2c_config *peripheral)
+{
+	u32 msg_xfer_cnt, wr_idx = 0;
+	struct gpi_multi_xfer *tx_multi_xfer = &peripheral->multi_xfer;
+
+	/*
+	 * In error case, need to unmap all messages based on the msg_idx_cnt.
+	 * Non-error case unmap all the processed messages.
+	 */
+	if (gi2c->err)
+		msg_xfer_cnt = tx_multi_xfer->msg_idx_cnt;
+	else
+		msg_xfer_cnt = tx_multi_xfer->irq_cnt * NUM_MSGS_PER_IRQ;
+
+	/* Unmap the processed DMA buffers based on the received interrupt count */
+	for (; tx_multi_xfer->unmap_msg_cnt < msg_xfer_cnt; tx_multi_xfer->unmap_msg_cnt++) {
+		if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs)
+			break;
+		wr_idx = tx_multi_xfer->unmap_msg_cnt % QCOM_GPI_MAX_NUM_MSGS;
+		geni_i2c_gpi_unmap(gi2c, &msgs[tx_multi_xfer->unmap_msg_cnt],
+				   tx_multi_xfer->dma_buf[wr_idx],
+				   tx_multi_xfer->dma_addr[wr_idx], NULL, NULL);
+		tx_multi_xfer->freed_msg_cnt++;
+	}
+}
+
+static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int cur_msg_idx,
 			struct dma_slave_config *config, dma_addr_t *dma_addr_p,
 			void **buf, unsigned int op, struct dma_chan *dma_chan)
 {
@@ -523,26 +576,49 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	enum dma_transfer_direction dma_dirn;
 	struct dma_async_tx_descriptor *desc;
 	int ret;
+	struct gpi_multi_xfer *gi2c_gpi_xfer;
+	dma_cookie_t cookie;
 
 	peripheral = config->peripheral_config;
-
-	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
-	if (!dma_buf)
+	gi2c_gpi_xfer = &peripheral->multi_xfer;
+	gi2c_gpi_xfer->msg_idx_cnt = cur_msg_idx;
+	dma_buf = gi2c_gpi_xfer->dma_buf[gi2c_gpi_xfer->buf_idx];
+	addr = gi2c_gpi_xfer->dma_addr[gi2c_gpi_xfer->buf_idx];
+
+	dma_buf = i2c_get_dma_safe_msg_buf(&msgs[gi2c_gpi_xfer->msg_idx_cnt], 1);
+	if (!dma_buf) {
+		gi2c->err = -ENOMEM;
 		return -ENOMEM;
+	}
 
 	if (op == I2C_WRITE)
 		map_dirn = DMA_TO_DEVICE;
 	else
 		map_dirn = DMA_FROM_DEVICE;
 
-	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
+	addr = dma_map_single(gi2c->se.dev->parent,
+			      dma_buf, msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
+			      map_dirn);
 	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
-		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
+		i2c_put_dma_safe_msg_buf(dma_buf, &msgs[gi2c_gpi_xfer->msg_idx_cnt],
+					 false);
+		gi2c->err = -ENOMEM;
 		return -ENOMEM;
 	}
 
+	if (gi2c->is_tx_multi_xfer) {
+		if (((gi2c_gpi_xfer->msg_idx_cnt + 1) % NUM_MSGS_PER_IRQ))
+			peripheral->flags |= QCOM_GPI_BLOCK_EVENT_IRQ;
+		else
+			peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
+
+		/* BEI bit to be cleared for last TRE */
+		if (gi2c_gpi_xfer->msg_idx_cnt == gi2c->num_msgs - 1)
+			peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
+	}
+
 	/* set the length as message for rx txn */
-	peripheral->rx_len = msg->len;
+	peripheral->rx_len = msgs[gi2c_gpi_xfer->msg_idx_cnt].len;
 	peripheral->op = op;
 
 	ret = dmaengine_slave_config(dma_chan, config);
@@ -560,25 +636,56 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	else
 		dma_dirn = DMA_DEV_TO_MEM;
 
-	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
+	desc = dmaengine_prep_slave_single(dma_chan, addr,
+					   msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
+					   dma_dirn, flags);
 	if (!desc) {
 		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
-		ret = -EIO;
+		gi2c->err = -EIO;
 		goto err_config;
 	}
 
 	desc->callback_result = i2c_gpi_cb_result;
 	desc->callback_param = gi2c;
 
-	dmaengine_submit(desc);
-	*buf = dma_buf;
-	*dma_addr_p = addr;
+	if (!((msgs[cur_msg_idx].flags & I2C_M_RD) && op == I2C_WRITE)) {
+		gi2c_gpi_xfer->msg_idx_cnt++;
+		gi2c_gpi_xfer->buf_idx = (cur_msg_idx + 1) % QCOM_GPI_MAX_NUM_MSGS;
+	}
+	cookie = dmaengine_submit(desc);
+	if (dma_submit_error(cookie)) {
+		dev_err(gi2c->se.dev,
+			"%s: dmaengine_submit failed (%d)\n", __func__, cookie);
+		return -EINVAL;
+	}
 
+	if (gi2c->is_tx_multi_xfer) {
+		dma_async_issue_pending(gi2c->tx_c);
+		if ((cur_msg_idx == (gi2c->num_msgs - 1)) ||
+		    (gi2c_gpi_xfer->msg_idx_cnt >=
+		     QCOM_GPI_MAX_NUM_MSGS + gi2c_gpi_xfer->freed_msg_cnt)) {
+			ret = gpi_multi_desc_process(gi2c->se.dev, gi2c_gpi_xfer,
+						     gi2c->num_msgs, XFER_TIMEOUT,
+						     &gi2c->done);
+			if (ret) {
+				dev_dbg(gi2c->se.dev,
+					"I2C multi write msg transfer timeout: %d\n",
+					ret);
+				gi2c->err = -ETIMEDOUT;
+				goto err_config;
+			}
+		}
+	} else {
+		/* Non multi descriptor message transfer */
+		*buf = dma_buf;
+		*dma_addr_p = addr;
+	}
 	return 0;
 
 err_config:
-	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
-	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
+	dma_unmap_single(gi2c->se.dev->parent, addr,
+			 msgs[cur_msg_idx].len, map_dirn);
+	i2c_put_dma_safe_msg_buf(dma_buf, &msgs[cur_msg_idx], false);
 	return ret;
 }
 
@@ -590,6 +697,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 	unsigned long time_left;
 	dma_addr_t tx_addr, rx_addr;
 	void *tx_buf = NULL, *rx_buf = NULL;
+	struct gpi_multi_xfer *tx_multi_xfer;
 	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
 
 	config.peripheral_config = &peripheral;
@@ -603,6 +711,39 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 	peripheral.set_config = 1;
 	peripheral.multi_msg = false;
 
+	gi2c->gpi_config = &peripheral;
+	gi2c->num_msgs = num;
+	gi2c->is_tx_multi_xfer = false;
+	gi2c->tx_irq_cnt = 0;
+
+	tx_multi_xfer = &peripheral.multi_xfer;
+	tx_multi_xfer->msg_idx_cnt = 0;
+	tx_multi_xfer->buf_idx = 0;
+	tx_multi_xfer->unmap_msg_cnt = 0;
+	tx_multi_xfer->freed_msg_cnt = 0;
+	tx_multi_xfer->irq_msg_cnt = 0;
+	tx_multi_xfer->irq_cnt = 0;
+
+	/*
+	 * If number of write messages are four and higher then
+	 * configure hardware for multi descriptor transfers with BEI.
+	 */
+	if (num >= MIN_NUM_OF_MSGS_MULTI_DESC) {
+		gi2c->is_tx_multi_xfer = true;
+		for (i = 0; i < num; i++) {
+			if (msgs[i].flags & I2C_M_RD) {
+				/*
+				 * Multi descriptor transfer with BEI
+				 * support is enabled for write transfers.
+				 * Add BEI optimization support for read
+				 * transfers later.
+				 */
+				gi2c->is_tx_multi_xfer = false;
+				break;
+			}
+		}
+	}
+
 	for (i = 0; i < num; i++) {
 		gi2c->cur = &msgs[i];
 		gi2c->err = 0;
@@ -613,14 +754,16 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 			peripheral.stretch = 1;
 
 		peripheral.addr = msgs[i].addr;
+		if (i > 0 && (!(msgs[i].flags & I2C_M_RD)))
+			peripheral.multi_msg = false;
 
-		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
+		ret =  geni_i2c_gpi(gi2c, msgs, i, &config,
 				    &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
 		if (ret)
 			goto err;
 
 		if (msgs[i].flags & I2C_M_RD) {
-			ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
+			ret =  geni_i2c_gpi(gi2c, msgs, i, &config,
 					    &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
 			if (ret)
 				goto err;
@@ -628,18 +771,28 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 			dma_async_issue_pending(gi2c->rx_c);
 		}
 
-		dma_async_issue_pending(gi2c->tx_c);
-
-		time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
-		if (!time_left)
-			gi2c->err = -ETIMEDOUT;
+		if (!gi2c->is_tx_multi_xfer) {
+			dma_async_issue_pending(gi2c->tx_c);
+			time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
+			if (!time_left) {
+				dev_err(gi2c->se.dev, "%s:I2C timeout\n", __func__);
+				gi2c->err = -ETIMEDOUT;
+			}
+		}
 
 		if (gi2c->err) {
 			ret = gi2c->err;
 			goto err;
 		}
 
-		geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
+		if (!gi2c->is_tx_multi_xfer) {
+			geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
+		} else {
+			if (gi2c->tx_irq_cnt != tx_multi_xfer->irq_cnt) {
+				gi2c->tx_irq_cnt = tx_multi_xfer->irq_cnt;
+				gpi_i2c_multi_desc_unmap(gi2c, msgs, &peripheral);
+			}
+		}
 	}
 
 	return num;
@@ -648,7 +801,11 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
 	dev_err(gi2c->se.dev, "GPI transfer failed: %d\n", ret);
 	dmaengine_terminate_sync(gi2c->rx_c);
 	dmaengine_terminate_sync(gi2c->tx_c);
-	geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
+	if (gi2c->is_tx_multi_xfer)
+		gpi_i2c_multi_desc_unmap(gi2c, msgs, &peripheral);
+	else
+		geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
+
 	return ret;
 }
 
-- 
2.17.1

Hi Jyothi,

kernel test robot noticed the following build errors:

[auto build test ERROR on 55bcd2e0d04c1171d382badef1def1fd04ef66c5]

url:    https://github.com/intel-lab-lkp/linux/commits/Jyothi-Kumar-Seerapu/dt-bindings-dmaengine-qcom-gpi-Add-additional-arg-to-dma-cell-property/20241015-202637
base:   55bcd2e0d04c1171d382badef1def1fd04ef66c5
patch link:    https://lore.kernel.org/r/20241015120750.21217-6-quic_jseerapu%40quicinc.com
patch subject: [PATCH v1 5/5] i2c: i2c-qcom-geni: Add Block event interrupt support
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20241019/202410191055.bi1pWTAY-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241019/202410191055.bi1pWTAY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410191055.bi1pWTAY-lkp@intel.com/

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "gpi_multi_desc_process" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
   WARNING: unmet direct dependencies detected for OMAP2PLUS_MBOX
   Depends on [n]: MAILBOX [=y] && (ARCH_OMAP2PLUS || ARCH_K3)
   Selected by [m]:
   - TI_K3_M4_REMOTEPROC [=m] && REMOTEPROC [=y] && (ARCH_K3 || COMPILE_TEST [=y])

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On 10/19/2024 8:42 AM, kernel test robot wrote:
> Hi Jyothi,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on 55bcd2e0d04c1171d382badef1def1fd04ef66c5]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Jyothi-Kumar-Seerapu/dt-bindings-dmaengine-qcom-gpi-Add-additional-arg-to-dma-cell-property/20241015-202637
> base:   55bcd2e0d04c1171d382badef1def1fd04ef66c5
> patch link:    https://lore.kernel.org/r/20241015120750.21217-6-quic_jseerapu%40quicinc.com
> patch subject: [PATCH v1 5/5] i2c: i2c-qcom-geni: Add Block event interrupt support
> config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20241019/202410191055.bi1pWTAY-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241019/202410191055.bi1pWTAY-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410191055.bi1pWTAY-lkp@intel.com/
> 
> All errors (new ones prefixed by >>, old ones prefixed by <<):
> 
>>> ERROR: modpost: "gpi_multi_desc_process" [drivers/i2c/busses/i2c-qcom-geni.ko] undefined!
> 
> Kconfig warnings: (for reference only)
>     WARNING: unmet direct dependencies detected for GET_FREE_REGION
>     Depends on [n]: SPARSEMEM [=n]
>     Selected by [m]:
>     - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]
>     WARNING: unmet direct dependencies detected for OMAP2PLUS_MBOX
>     Depends on [n]: MAILBOX [=y] && (ARCH_OMAP2PLUS || ARCH_K3)
>     Selected by [m]:
>     - TI_K3_M4_REMOTEPROC [=m] && REMOTEPROC [=y] && (ARCH_K3 || COMPILE_TEST [=y])
> 

Fixed the reported issue in V2 patch.

Regards,
JyothiKumar

Hi Jyothi,

kernel test robot noticed the following build errors:

[auto build test ERROR on 55bcd2e0d04c1171d382badef1def1fd04ef66c5]

url:    https://github.com/intel-lab-lkp/linux/commits/Jyothi-Kumar-Seerapu/dt-bindings-dmaengine-qcom-gpi-Add-additional-arg-to-dma-cell-property/20241015-202637
base:   55bcd2e0d04c1171d382badef1def1fd04ef66c5
patch link:    https://lore.kernel.org/r/20241015120750.21217-6-quic_jseerapu%40quicinc.com
patch subject: [PATCH v1 5/5] i2c: i2c-qcom-geni: Add Block event interrupt support
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20241019/202410190549.hGAfByqg-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241019/202410190549.hGAfByqg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410190549.hGAfByqg-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/i2c/busses/i2c-qcom-geni.c:562:8: error: incompatible pointer to integer conversion passing 'dma_addr_t *' (aka 'unsigned long long *') to parameter of type 'dma_addr_t' (aka 'unsigned long long'); dereference with * [-Wint-conversion]
     562 |                                    tx_multi_xfer->dma_addr[wr_idx], NULL, NULL);
         |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                                    *
   drivers/i2c/busses/i2c-qcom-geni.c:519:36: note: passing argument to parameter 'tx_addr' here
     519 |                                void *tx_buf, dma_addr_t tx_addr,
         |                                                         ^
>> drivers/i2c/busses/i2c-qcom-geni.c:562:47: error: incompatible pointer to integer conversion passing 'void *' to parameter of type 'dma_addr_t' (aka 'unsigned long long') [-Wint-conversion]
     562 |                                    tx_multi_xfer->dma_addr[wr_idx], NULL, NULL);
         |                                                                           ^~~~
   include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
       8 | #define NULL ((void *)0)
         |              ^~~~~~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:520:36: note: passing argument to parameter 'rx_addr' here
     520 |                                void *rx_buf, dma_addr_t rx_addr)
         |                                                         ^
>> drivers/i2c/busses/i2c-qcom-geni.c:586:7: error: incompatible pointer to integer conversion assigning to 'dma_addr_t' (aka 'unsigned long long') from 'dma_addr_t *' (aka 'unsigned long long *'); dereference with * [-Wint-conversion]
     586 |         addr = gi2c_gpi_xfer->dma_addr[gi2c_gpi_xfer->buf_idx];
         |              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         |                *
   3 errors generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for OMAP2PLUS_MBOX
   Depends on [n]: MAILBOX [=y] && (ARCH_OMAP2PLUS || ARCH_K3)
   Selected by [y]:
   - TI_K3_M4_REMOTEPROC [=y] && REMOTEPROC [=y] && (ARCH_K3 || COMPILE_TEST [=y])


vim +562 drivers/i2c/busses/i2c-qcom-geni.c

   532	
   533	/**
   534	 * gpi_i2c_multi_desc_unmap() - unmaps the buffers post multi message TX transfers
   535	 * @dev: pointer to the corresponding dev node
   536	 * @gi2c: i2c dev handle
   537	 * @msgs: i2c messages array
   538	 * @peripheral: pointer to the gpi_i2c_config
   539	 */
   540	static void gpi_i2c_multi_desc_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
   541					     struct gpi_i2c_config *peripheral)
   542	{
   543		u32 msg_xfer_cnt, wr_idx = 0;
   544		struct gpi_multi_xfer *tx_multi_xfer = &peripheral->multi_xfer;
   545	
   546		/*
   547		 * In error case, need to unmap all messages based on the msg_idx_cnt.
   548		 * Non-error case unmap all the processed messages.
   549		 */
   550		if (gi2c->err)
   551			msg_xfer_cnt = tx_multi_xfer->msg_idx_cnt;
   552		else
   553			msg_xfer_cnt = tx_multi_xfer->irq_cnt * NUM_MSGS_PER_IRQ;
   554	
   555		/* Unmap the processed DMA buffers based on the received interrupt count */
   556		for (; tx_multi_xfer->unmap_msg_cnt < msg_xfer_cnt; tx_multi_xfer->unmap_msg_cnt++) {
   557			if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs)
   558				break;
   559			wr_idx = tx_multi_xfer->unmap_msg_cnt % QCOM_GPI_MAX_NUM_MSGS;
   560			geni_i2c_gpi_unmap(gi2c, &msgs[tx_multi_xfer->unmap_msg_cnt],
   561					   tx_multi_xfer->dma_buf[wr_idx],
 > 562					   tx_multi_xfer->dma_addr[wr_idx], NULL, NULL);
   563			tx_multi_xfer->freed_msg_cnt++;
   564		}
   565	}
   566	
   567	static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int cur_msg_idx,
   568				struct dma_slave_config *config, dma_addr_t *dma_addr_p,
   569				void **buf, unsigned int op, struct dma_chan *dma_chan)
   570	{
   571		struct gpi_i2c_config *peripheral;
   572		unsigned int flags;
   573		void *dma_buf;
   574		dma_addr_t addr;
   575		enum dma_data_direction map_dirn;
   576		enum dma_transfer_direction dma_dirn;
   577		struct dma_async_tx_descriptor *desc;
   578		int ret;
   579		struct gpi_multi_xfer *gi2c_gpi_xfer;
   580		dma_cookie_t cookie;
   581	
   582		peripheral = config->peripheral_config;
   583		gi2c_gpi_xfer = &peripheral->multi_xfer;
   584		gi2c_gpi_xfer->msg_idx_cnt = cur_msg_idx;
   585		dma_buf = gi2c_gpi_xfer->dma_buf[gi2c_gpi_xfer->buf_idx];
 > 586		addr = gi2c_gpi_xfer->dma_addr[gi2c_gpi_xfer->buf_idx];
   587	
   588		dma_buf = i2c_get_dma_safe_msg_buf(&msgs[gi2c_gpi_xfer->msg_idx_cnt], 1);
   589		if (!dma_buf) {
   590			gi2c->err = -ENOMEM;
   591			return -ENOMEM;
   592		}
   593	
   594		if (op == I2C_WRITE)
   595			map_dirn = DMA_TO_DEVICE;
   596		else
   597			map_dirn = DMA_FROM_DEVICE;
   598	
   599		addr = dma_map_single(gi2c->se.dev->parent,
   600				      dma_buf, msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
   601				      map_dirn);
   602		if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
   603			i2c_put_dma_safe_msg_buf(dma_buf, &msgs[gi2c_gpi_xfer->msg_idx_cnt],
   604						 false);
   605			gi2c->err = -ENOMEM;
   606			return -ENOMEM;
   607		}
   608	
   609		if (gi2c->is_tx_multi_xfer) {
   610			if (((gi2c_gpi_xfer->msg_idx_cnt + 1) % NUM_MSGS_PER_IRQ))
   611				peripheral->flags |= QCOM_GPI_BLOCK_EVENT_IRQ;
   612			else
   613				peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
   614	
   615			/* BEI bit to be cleared for last TRE */
   616			if (gi2c_gpi_xfer->msg_idx_cnt == gi2c->num_msgs - 1)
   617				peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
   618		}
   619	
   620		/* set the length as message for rx txn */
   621		peripheral->rx_len = msgs[gi2c_gpi_xfer->msg_idx_cnt].len;
   622		peripheral->op = op;
   623	
   624		ret = dmaengine_slave_config(dma_chan, config);
   625		if (ret) {
   626			dev_err(gi2c->se.dev, "dma config error: %d for op:%d\n", ret, op);
   627			goto err_config;
   628		}
   629	
   630		peripheral->set_config = 0;
   631		peripheral->multi_msg = true;
   632		flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
   633	
   634		if (op == I2C_WRITE)
   635			dma_dirn = DMA_MEM_TO_DEV;
   636		else
   637			dma_dirn = DMA_DEV_TO_MEM;
   638	
   639		desc = dmaengine_prep_slave_single(dma_chan, addr,
   640						   msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
   641						   dma_dirn, flags);
   642		if (!desc) {
   643			dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
   644			gi2c->err = -EIO;
   645			goto err_config;
   646		}
   647	
   648		desc->callback_result = i2c_gpi_cb_result;
   649		desc->callback_param = gi2c;
   650	
   651		if (!((msgs[cur_msg_idx].flags & I2C_M_RD) && op == I2C_WRITE)) {
   652			gi2c_gpi_xfer->msg_idx_cnt++;
   653			gi2c_gpi_xfer->buf_idx = (cur_msg_idx + 1) % QCOM_GPI_MAX_NUM_MSGS;
   654		}
   655		cookie = dmaengine_submit(desc);
   656		if (dma_submit_error(cookie)) {
   657			dev_err(gi2c->se.dev,
   658				"%s: dmaengine_submit failed (%d)\n", __func__, cookie);
   659			return -EINVAL;
   660		}
   661	
   662		if (gi2c->is_tx_multi_xfer) {
   663			dma_async_issue_pending(gi2c->tx_c);
   664			if ((cur_msg_idx == (gi2c->num_msgs - 1)) ||
   665			    (gi2c_gpi_xfer->msg_idx_cnt >=
   666			     QCOM_GPI_MAX_NUM_MSGS + gi2c_gpi_xfer->freed_msg_cnt)) {
   667				ret = gpi_multi_desc_process(gi2c->se.dev, gi2c_gpi_xfer,
   668							     gi2c->num_msgs, XFER_TIMEOUT,
   669							     &gi2c->done);
   670				if (ret) {
   671					dev_dbg(gi2c->se.dev,
   672						"I2C multi write msg transfer timeout: %d\n",
   673						ret);
   674					gi2c->err = -ETIMEDOUT;
   675					goto err_config;
   676				}
   677			}
   678		} else {
   679			/* Non multi descriptor message transfer */
   680			*buf = dma_buf;
   681			*dma_addr_p = addr;
   682		}
   683		return 0;
   684	
   685	err_config:
   686		dma_unmap_single(gi2c->se.dev->parent, addr,
   687				 msgs[cur_msg_idx].len, map_dirn);
   688		i2c_put_dma_safe_msg_buf(dma_buf, &msgs[cur_msg_idx], false);
   689		return ret;
   690	}
   691	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On 10/19/2024 3:41 AM, kernel test robot wrote:
> Hi Jyothi,
> 
> kernel test robot noticed the following build errors:
> 
> [auto build test ERROR on 55bcd2e0d04c1171d382badef1def1fd04ef66c5]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Jyothi-Kumar-Seerapu/dt-bindings-dmaengine-qcom-gpi-Add-additional-arg-to-dma-cell-property/20241015-202637
> base:   55bcd2e0d04c1171d382badef1def1fd04ef66c5
> patch link:    https://lore.kernel.org/r/20241015120750.21217-6-quic_jseerapu%40quicinc.com
> patch subject: [PATCH v1 5/5] i2c: i2c-qcom-geni: Add Block event interrupt support
> config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20241019/202410190549.hGAfByqg-lkp@intel.com/config)
> compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241019/202410190549.hGAfByqg-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202410190549.hGAfByqg-lkp@intel.com/
> 
> All errors (new ones prefixed by >>):
> 
>>> drivers/i2c/busses/i2c-qcom-geni.c:562:8: error: incompatible pointer to integer conversion passing 'dma_addr_t *' (aka 'unsigned long long *') to parameter of type 'dma_addr_t' (aka 'unsigned long long'); dereference with * [-Wint-conversion]
>       562 |                                    tx_multi_xfer->dma_addr[wr_idx], NULL, NULL);
>           |                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>           |                                    *
>     drivers/i2c/busses/i2c-qcom-geni.c:519:36: note: passing argument to parameter 'tx_addr' here
>       519 |                                void *tx_buf, dma_addr_t tx_addr,
>           |                                                         ^
>>> drivers/i2c/busses/i2c-qcom-geni.c:562:47: error: incompatible pointer to integer conversion passing 'void *' to parameter of type 'dma_addr_t' (aka 'unsigned long long') [-Wint-conversion]
>       562 |                                    tx_multi_xfer->dma_addr[wr_idx], NULL, NULL);
>           |                                                                           ^~~~
>     include/linux/stddef.h:8:14: note: expanded from macro 'NULL'
>         8 | #define NULL ((void *)0)
>           |              ^~~~~~~~~~~
>     drivers/i2c/busses/i2c-qcom-geni.c:520:36: note: passing argument to parameter 'rx_addr' here
>       520 |                                void *rx_buf, dma_addr_t rx_addr)
>           |                                                         ^
>>> drivers/i2c/busses/i2c-qcom-geni.c:586:7: error: incompatible pointer to integer conversion assigning to 'dma_addr_t' (aka 'unsigned long long') from 'dma_addr_t *' (aka 'unsigned long long *'); dereference with * [-Wint-conversion]
>       586 |         addr = gi2c_gpi_xfer->dma_addr[gi2c_gpi_xfer->buf_idx];
>           |              ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>           |                *
>     3 errors generated.

Fixed the reported issues which are comiltation warnings in V2 patch.

> 
> Kconfig warnings: (for reference only)
>     WARNING: unmet direct dependencies detected for OMAP2PLUS_MBOX
>     Depends on [n]: MAILBOX [=y] && (ARCH_OMAP2PLUS || ARCH_K3)
>     Selected by [y]:
>     - TI_K3_M4_REMOTEPROC [=y] && REMOTEPROC [=y] && (ARCH_K3 || COMPILE_TEST [=y])
> 
> 
> vim +562 drivers/i2c/busses/i2c-qcom-geni.c
> 
>     532	
>     533	/**
>     534	 * gpi_i2c_multi_desc_unmap() - unmaps the buffers post multi message TX transfers
>     535	 * @dev: pointer to the corresponding dev node
>     536	 * @gi2c: i2c dev handle
>     537	 * @msgs: i2c messages array
>     538	 * @peripheral: pointer to the gpi_i2c_config
>     539	 */
>     540	static void gpi_i2c_multi_desc_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
>     541					     struct gpi_i2c_config *peripheral)
>     542	{
>     543		u32 msg_xfer_cnt, wr_idx = 0;
>     544		struct gpi_multi_xfer *tx_multi_xfer = &peripheral->multi_xfer;
>     545	
>     546		/*
>     547		 * In error case, need to unmap all messages based on the msg_idx_cnt.
>     548		 * Non-error case unmap all the processed messages.
>     549		 */
>     550		if (gi2c->err)
>     551			msg_xfer_cnt = tx_multi_xfer->msg_idx_cnt;
>     552		else
>     553			msg_xfer_cnt = tx_multi_xfer->irq_cnt * NUM_MSGS_PER_IRQ;
>     554	
>     555		/* Unmap the processed DMA buffers based on the received interrupt count */
>     556		for (; tx_multi_xfer->unmap_msg_cnt < msg_xfer_cnt; tx_multi_xfer->unmap_msg_cnt++) {
>     557			if (tx_multi_xfer->unmap_msg_cnt == gi2c->num_msgs)
>     558				break;
>     559			wr_idx = tx_multi_xfer->unmap_msg_cnt % QCOM_GPI_MAX_NUM_MSGS;
>     560			geni_i2c_gpi_unmap(gi2c, &msgs[tx_multi_xfer->unmap_msg_cnt],
>     561					   tx_multi_xfer->dma_buf[wr_idx],
>   > 562					   tx_multi_xfer->dma_addr[wr_idx], NULL, NULL);
>     563			tx_multi_xfer->freed_msg_cnt++;
>     564		}
>     565	}
>     566	
>     567	static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int cur_msg_idx,
>     568				struct dma_slave_config *config, dma_addr_t *dma_addr_p,
>     569				void **buf, unsigned int op, struct dma_chan *dma_chan)
>     570	{
>     571		struct gpi_i2c_config *peripheral;
>     572		unsigned int flags;
>     573		void *dma_buf;
>     574		dma_addr_t addr;
>     575		enum dma_data_direction map_dirn;
>     576		enum dma_transfer_direction dma_dirn;
>     577		struct dma_async_tx_descriptor *desc;
>     578		int ret;
>     579		struct gpi_multi_xfer *gi2c_gpi_xfer;
>     580		dma_cookie_t cookie;
>     581	
>     582		peripheral = config->peripheral_config;
>     583		gi2c_gpi_xfer = &peripheral->multi_xfer;
>     584		gi2c_gpi_xfer->msg_idx_cnt = cur_msg_idx;
>     585		dma_buf = gi2c_gpi_xfer->dma_buf[gi2c_gpi_xfer->buf_idx];
>   > 586		addr = gi2c_gpi_xfer->dma_addr[gi2c_gpi_xfer->buf_idx];
>     587	
>     588		dma_buf = i2c_get_dma_safe_msg_buf(&msgs[gi2c_gpi_xfer->msg_idx_cnt], 1);
>     589		if (!dma_buf) {
>     590			gi2c->err = -ENOMEM;
>     591			return -ENOMEM;
>     592		}
>     593	
>     594		if (op == I2C_WRITE)
>     595			map_dirn = DMA_TO_DEVICE;
>     596		else
>     597			map_dirn = DMA_FROM_DEVICE;
>     598	
>     599		addr = dma_map_single(gi2c->se.dev->parent,
>     600				      dma_buf, msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
>     601				      map_dirn);
>     602		if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
>     603			i2c_put_dma_safe_msg_buf(dma_buf, &msgs[gi2c_gpi_xfer->msg_idx_cnt],
>     604						 false);
>     605			gi2c->err = -ENOMEM;
>     606			return -ENOMEM;
>     607		}
>     608	
>     609		if (gi2c->is_tx_multi_xfer) {
>     610			if (((gi2c_gpi_xfer->msg_idx_cnt + 1) % NUM_MSGS_PER_IRQ))
>     611				peripheral->flags |= QCOM_GPI_BLOCK_EVENT_IRQ;
>     612			else
>     613				peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
>     614	
>     615			/* BEI bit to be cleared for last TRE */
>     616			if (gi2c_gpi_xfer->msg_idx_cnt == gi2c->num_msgs - 1)
>     617				peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
>     618		}
>     619	
>     620		/* set the length as message for rx txn */
>     621		peripheral->rx_len = msgs[gi2c_gpi_xfer->msg_idx_cnt].len;
>     622		peripheral->op = op;
>     623	
>     624		ret = dmaengine_slave_config(dma_chan, config);
>     625		if (ret) {
>     626			dev_err(gi2c->se.dev, "dma config error: %d for op:%d\n", ret, op);
>     627			goto err_config;
>     628		}
>     629	
>     630		peripheral->set_config = 0;
>     631		peripheral->multi_msg = true;
>     632		flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
>     633	
>     634		if (op == I2C_WRITE)
>     635			dma_dirn = DMA_MEM_TO_DEV;
>     636		else
>     637			dma_dirn = DMA_DEV_TO_MEM;
>     638	
>     639		desc = dmaengine_prep_slave_single(dma_chan, addr,
>     640						   msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
>     641						   dma_dirn, flags);
>     642		if (!desc) {
>     643			dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
>     644			gi2c->err = -EIO;
>     645			goto err_config;
>     646		}
>     647	
>     648		desc->callback_result = i2c_gpi_cb_result;
>     649		desc->callback_param = gi2c;
>     650	
>     651		if (!((msgs[cur_msg_idx].flags & I2C_M_RD) && op == I2C_WRITE)) {
>     652			gi2c_gpi_xfer->msg_idx_cnt++;
>     653			gi2c_gpi_xfer->buf_idx = (cur_msg_idx + 1) % QCOM_GPI_MAX_NUM_MSGS;
>     654		}
>     655		cookie = dmaengine_submit(desc);
>     656		if (dma_submit_error(cookie)) {
>     657			dev_err(gi2c->se.dev,
>     658				"%s: dmaengine_submit failed (%d)\n", __func__, cookie);
>     659			return -EINVAL;
>     660		}
>     661	
>     662		if (gi2c->is_tx_multi_xfer) {
>     663			dma_async_issue_pending(gi2c->tx_c);
>     664			if ((cur_msg_idx == (gi2c->num_msgs - 1)) ||
>     665			    (gi2c_gpi_xfer->msg_idx_cnt >=
>     666			     QCOM_GPI_MAX_NUM_MSGS + gi2c_gpi_xfer->freed_msg_cnt)) {
>     667				ret = gpi_multi_desc_process(gi2c->se.dev, gi2c_gpi_xfer,
>     668							     gi2c->num_msgs, XFER_TIMEOUT,
>     669							     &gi2c->done);
>     670				if (ret) {
>     671					dev_dbg(gi2c->se.dev,
>     672						"I2C multi write msg transfer timeout: %d\n",
>     673						ret);
>     674					gi2c->err = -ETIMEDOUT;
>     675					goto err_config;
>     676				}
>     677			}
>     678		} else {
>     679			/* Non multi descriptor message transfer */
>     680			*buf = dma_buf;
>     681			*dma_addr_p = addr;
>     682		}
>     683		return 0;
>     684	
>     685	err_config:
>     686		dma_unmap_single(gi2c->se.dev->parent, addr,
>     687				 msgs[cur_msg_idx].len, map_dirn);
>     688		i2c_put_dma_safe_msg_buf(dma_buf, &msgs[cur_msg_idx], false);
>     689		return ret;
>     690	}
>     691	
>

Hi Jyothi,

...

> @@ -523,26 +576,49 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>  	enum dma_transfer_direction dma_dirn;
>  	struct dma_async_tx_descriptor *desc;
>  	int ret;
> +	struct gpi_multi_xfer *gi2c_gpi_xfer;
> +	dma_cookie_t cookie;
>  
>  	peripheral = config->peripheral_config;
> -
> -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
> -	if (!dma_buf)
> +	gi2c_gpi_xfer = &peripheral->multi_xfer;
> +	gi2c_gpi_xfer->msg_idx_cnt = cur_msg_idx;
> +	dma_buf = gi2c_gpi_xfer->dma_buf[gi2c_gpi_xfer->buf_idx];
> +	addr = gi2c_gpi_xfer->dma_addr[gi2c_gpi_xfer->buf_idx];
> +
> +	dma_buf = i2c_get_dma_safe_msg_buf(&msgs[gi2c_gpi_xfer->msg_idx_cnt], 1);
> +	if (!dma_buf) {
> +		gi2c->err = -ENOMEM;
>  		return -ENOMEM;
> +	}
>  
>  	if (op == I2C_WRITE)
>  		map_dirn = DMA_TO_DEVICE;
>  	else
>  		map_dirn = DMA_FROM_DEVICE;
>  
> -	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
> +	addr = dma_map_single(gi2c->se.dev->parent,
> +			      dma_buf, msgs[gi2c_gpi_xfer->msg_idx_cnt].len,

You could save msgs[gi2c_gpi_xfer->msg_idx_cnt] in a separate
variable to avoid this extra indexing.

> +			      map_dirn);
>  	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
> -		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
> +		i2c_put_dma_safe_msg_buf(dma_buf, &msgs[gi2c_gpi_xfer->msg_idx_cnt],
> +					 false);
> +		gi2c->err = -ENOMEM;
>  		return -ENOMEM;
>  	}
>  
> +	if (gi2c->is_tx_multi_xfer) {
> +		if (((gi2c_gpi_xfer->msg_idx_cnt + 1) % NUM_MSGS_PER_IRQ))
> +			peripheral->flags |= QCOM_GPI_BLOCK_EVENT_IRQ;
> +		else
> +			peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
> +
> +		/* BEI bit to be cleared for last TRE */
> +		if (gi2c_gpi_xfer->msg_idx_cnt == gi2c->num_msgs - 1)
> +			peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
> +	}
> +
>  	/* set the length as message for rx txn */
> -	peripheral->rx_len = msg->len;
> +	peripheral->rx_len = msgs[gi2c_gpi_xfer->msg_idx_cnt].len;
>  	peripheral->op = op;
>  
>  	ret = dmaengine_slave_config(dma_chan, config);
> @@ -560,25 +636,56 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>  	else
>  		dma_dirn = DMA_DEV_TO_MEM;
>  
> -	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
> +	desc = dmaengine_prep_slave_single(dma_chan, addr,
> +					   msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
> +					   dma_dirn, flags);
>  	if (!desc) {
>  		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
> -		ret = -EIO;
> +		gi2c->err = -EIO;
>  		goto err_config;
>  	}
>  
>  	desc->callback_result = i2c_gpi_cb_result;
>  	desc->callback_param = gi2c;
>  
> -	dmaengine_submit(desc);
> -	*buf = dma_buf;
> -	*dma_addr_p = addr;
> +	if (!((msgs[cur_msg_idx].flags & I2C_M_RD) && op == I2C_WRITE)) {
> +		gi2c_gpi_xfer->msg_idx_cnt++;
> +		gi2c_gpi_xfer->buf_idx = (cur_msg_idx + 1) % QCOM_GPI_MAX_NUM_MSGS;
> +	}
> +	cookie = dmaengine_submit(desc);
> +	if (dma_submit_error(cookie)) {
> +		dev_err(gi2c->se.dev,
> +			"%s: dmaengine_submit failed (%d)\n", __func__, cookie);
> +		return -EINVAL;

goto err_config?

> +	}
>  
> +	if (gi2c->is_tx_multi_xfer) {
> +		dma_async_issue_pending(gi2c->tx_c);
> +		if ((cur_msg_idx == (gi2c->num_msgs - 1)) ||
> +		    (gi2c_gpi_xfer->msg_idx_cnt >=
> +		     QCOM_GPI_MAX_NUM_MSGS + gi2c_gpi_xfer->freed_msg_cnt)) {
> +			ret = gpi_multi_desc_process(gi2c->se.dev, gi2c_gpi_xfer,
> +						     gi2c->num_msgs, XFER_TIMEOUT,
> +						     &gi2c->done);
> +			if (ret) {
> +				dev_dbg(gi2c->se.dev,
> +					"I2C multi write msg transfer timeout: %d\n",
> +					ret);

if you are returning an error, then print an error.

> +				gi2c->err = -ETIMEDOUT;

gi2c->err = ret?

> +				goto err_config;
> +			}
> +		}
> +	} else {
> +		/* Non multi descriptor message transfer */
> +		*buf = dma_buf;
> +		*dma_addr_p = addr;
> +	}
>  	return 0;
>  
>  err_config:
> -	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
> -	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
> +	dma_unmap_single(gi2c->se.dev->parent, addr,
> +			 msgs[cur_msg_idx].len, map_dirn);
> +	i2c_put_dma_safe_msg_buf(dma_buf, &msgs[cur_msg_idx], false);
>  	return ret;

I would have one more label here:

   out:
	gi2c->err = ret;

	return ret;

in order to avoid always assigning twice

>  }
>  
> @@ -590,6 +697,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  	unsigned long time_left;
>  	dma_addr_t tx_addr, rx_addr;
>  	void *tx_buf = NULL, *rx_buf = NULL;
> +	struct gpi_multi_xfer *tx_multi_xfer;
>  	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>  
>  	config.peripheral_config = &peripheral;
> @@ -603,6 +711,39 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  	peripheral.set_config = 1;
>  	peripheral.multi_msg = false;
>  
> +	gi2c->gpi_config = &peripheral;
> +	gi2c->num_msgs = num;
> +	gi2c->is_tx_multi_xfer = false;
> +	gi2c->tx_irq_cnt = 0;
> +
> +	tx_multi_xfer = &peripheral.multi_xfer;
> +	tx_multi_xfer->msg_idx_cnt = 0;
> +	tx_multi_xfer->buf_idx = 0;
> +	tx_multi_xfer->unmap_msg_cnt = 0;
> +	tx_multi_xfer->freed_msg_cnt = 0;
> +	tx_multi_xfer->irq_msg_cnt = 0;
> +	tx_multi_xfer->irq_cnt = 0;

you can initialize tx_multi_xfer to "{ };" to avoid all these
" = 0"

> +
> +	/*
> +	 * If number of write messages are four and higher then
> +	 * configure hardware for multi descriptor transfers with BEI.
> +	 */
> +	if (num >= MIN_NUM_OF_MSGS_MULTI_DESC) {
> +		gi2c->is_tx_multi_xfer = true;
> +		for (i = 0; i < num; i++) {
> +			if (msgs[i].flags & I2C_M_RD) {
> +				/*
> +				 * Multi descriptor transfer with BEI
> +				 * support is enabled for write transfers.
> +				 * Add BEI optimization support for read
> +				 * transfers later.
> +				 */
> +				gi2c->is_tx_multi_xfer = false;
> +				break;
> +			}
> +		}
> +	}
> +
>  	for (i = 0; i < num; i++) {
>  		gi2c->cur = &msgs[i];
>  		gi2c->err = 0;
> @@ -613,14 +754,16 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  			peripheral.stretch = 1;
>  
>  		peripheral.addr = msgs[i].addr;
> +		if (i > 0 && (!(msgs[i].flags & I2C_M_RD)))
> +			peripheral.multi_msg = false;
>  
> -		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
> +		ret =  geni_i2c_gpi(gi2c, msgs, i, &config,

what is the point of passing 'i' if you always refer to msgs[i]
in geni_i2c_gpi()?

>  				    &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
>  		if (ret)
>  			goto err;
>  
>  		if (msgs[i].flags & I2C_M_RD) {
> -			ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
> +			ret =  geni_i2c_gpi(gi2c, msgs, i, &config,
>  					    &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
>  			if (ret)
>  				goto err;
> @@ -628,18 +771,28 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>  			dma_async_issue_pending(gi2c->rx_c);
>  		}
>  
> -		dma_async_issue_pending(gi2c->tx_c);
> -
> -		time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
> -		if (!time_left)
> -			gi2c->err = -ETIMEDOUT;
> +		if (!gi2c->is_tx_multi_xfer) {
> +			dma_async_issue_pending(gi2c->tx_c);
> +			time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
> +			if (!time_left) {
> +				dev_err(gi2c->se.dev, "%s:I2C timeout\n", __func__);
> +				gi2c->err = -ETIMEDOUT;
> +			}
> +		}
>  
>  		if (gi2c->err) {
>  			ret = gi2c->err;
>  			goto err;
>  		}
>  
> -		geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
> +		if (!gi2c->is_tx_multi_xfer) {
> +			geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
> +		} else {
> +			if (gi2c->tx_irq_cnt != tx_multi_xfer->irq_cnt) {

   else if (...) {
   	...
   }

Andi

On 10/16/2024 8:36 PM, Andi Shyti wrote:
> Hi Jyothi,
> 
> ...
> 
>> @@ -523,26 +576,49 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>>   	enum dma_transfer_direction dma_dirn;
>>   	struct dma_async_tx_descriptor *desc;
>>   	int ret;
>> +	struct gpi_multi_xfer *gi2c_gpi_xfer;
>> +	dma_cookie_t cookie;
>>   
>>   	peripheral = config->peripheral_config;
>> -
>> -	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
>> -	if (!dma_buf)
>> +	gi2c_gpi_xfer = &peripheral->multi_xfer;
>> +	gi2c_gpi_xfer->msg_idx_cnt = cur_msg_idx;
>> +	dma_buf = gi2c_gpi_xfer->dma_buf[gi2c_gpi_xfer->buf_idx];
>> +	addr = gi2c_gpi_xfer->dma_addr[gi2c_gpi_xfer->buf_idx];
>> +
>> +	dma_buf = i2c_get_dma_safe_msg_buf(&msgs[gi2c_gpi_xfer->msg_idx_cnt], 1);
>> +	if (!dma_buf) {
>> +		gi2c->err = -ENOMEM;
>>   		return -ENOMEM;
>> +	}
>>   
>>   	if (op == I2C_WRITE)
>>   		map_dirn = DMA_TO_DEVICE;
>>   	else
>>   		map_dirn = DMA_FROM_DEVICE;
>>   
>> -	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
>> +	addr = dma_map_single(gi2c->se.dev->parent,
>> +			      dma_buf, msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
> 
> You could save msgs[gi2c_gpi_xfer->msg_idx_cnt] in a separate
> variable to avoid this extra indexing.
> 
Thanks Andi, moved gi2c_gpi_xfer->msg_idx_cnt to separate local variable.
>> +			      map_dirn);
>>   	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
>> -		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>> +		i2c_put_dma_safe_msg_buf(dma_buf, &msgs[gi2c_gpi_xfer->msg_idx_cnt],
>> +					 false);
>> +		gi2c->err = -ENOMEM;
>>   		return -ENOMEM;
>>   	}
>>   
>> +	if (gi2c->is_tx_multi_xfer) {
>> +		if (((gi2c_gpi_xfer->msg_idx_cnt + 1) % NUM_MSGS_PER_IRQ))
>> +			peripheral->flags |= QCOM_GPI_BLOCK_EVENT_IRQ;
>> +		else
>> +			peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
>> +
>> +		/* BEI bit to be cleared for last TRE */
>> +		if (gi2c_gpi_xfer->msg_idx_cnt == gi2c->num_msgs - 1)
>> +			peripheral->flags &= ~QCOM_GPI_BLOCK_EVENT_IRQ;
>> +	}
>> +
>>   	/* set the length as message for rx txn */
>> -	peripheral->rx_len = msg->len;
>> +	peripheral->rx_len = msgs[gi2c_gpi_xfer->msg_idx_cnt].len;
>>   	peripheral->op = op;
>>   
>>   	ret = dmaengine_slave_config(dma_chan, config);
>> @@ -560,25 +636,56 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>>   	else
>>   		dma_dirn = DMA_DEV_TO_MEM;
>>   
>> -	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
>> +	desc = dmaengine_prep_slave_single(dma_chan, addr,
>> +					   msgs[gi2c_gpi_xfer->msg_idx_cnt].len,
>> +					   dma_dirn, flags);
>>   	if (!desc) {
>>   		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
>> -		ret = -EIO;
>> +		gi2c->err = -EIO;
>>   		goto err_config;
>>   	}
>>   
>>   	desc->callback_result = i2c_gpi_cb_result;
>>   	desc->callback_param = gi2c;
>>   
>> -	dmaengine_submit(desc);
>> -	*buf = dma_buf;
>> -	*dma_addr_p = addr;
>> +	if (!((msgs[cur_msg_idx].flags & I2C_M_RD) && op == I2C_WRITE)) {
>> +		gi2c_gpi_xfer->msg_idx_cnt++;
>> +		gi2c_gpi_xfer->buf_idx = (cur_msg_idx + 1) % QCOM_GPI_MAX_NUM_MSGS;
>> +	}
>> +	cookie = dmaengine_submit(desc);
>> +	if (dma_submit_error(cookie)) {
>> +		dev_err(gi2c->se.dev,
>> +			"%s: dmaengine_submit failed (%d)\n", __func__, cookie);
>> +		return -EINVAL;
> 
> goto err_config?
yes, updated it.
> 
>> +	}
>>   
>> +	if (gi2c->is_tx_multi_xfer) {
>> +		dma_async_issue_pending(gi2c->tx_c);
>> +		if ((cur_msg_idx == (gi2c->num_msgs - 1)) ||
>> +		    (gi2c_gpi_xfer->msg_idx_cnt >=
>> +		     QCOM_GPI_MAX_NUM_MSGS + gi2c_gpi_xfer->freed_msg_cnt)) {
>> +			ret = gpi_multi_desc_process(gi2c->se.dev, gi2c_gpi_xfer,
>> +						     gi2c->num_msgs, XFER_TIMEOUT,
>> +						     &gi2c->done);
>> +			if (ret) {
>> +				dev_dbg(gi2c->se.dev,
>> +					"I2C multi write msg transfer timeout: %d\n",
>> +					ret);
> 
> if you are returning an error, then print an error.
sure, updated it to error in V2 patch.
> 
>> +				gi2c->err = -ETIMEDOUT;
> 
> gi2c->err = ret?
Yes in this case, ret is -ETIMEDOUT, so updated in V2 patch as 
gi2c->err= ret.
> 
>> +				goto err_config;
>> +			}
>> +		}
>> +	} else {
>> +		/* Non multi descriptor message transfer */
>> +		*buf = dma_buf;
>> +		*dma_addr_p = addr;
>> +	}
>>   	return 0;
>>   
>>   err_config:
>> -	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
>> -	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
>> +	dma_unmap_single(gi2c->se.dev->parent, addr,
>> +			 msgs[cur_msg_idx].len, map_dirn);
>> +	i2c_put_dma_safe_msg_buf(dma_buf, &msgs[cur_msg_idx], false);
>>   	return ret;
> 
> I would have one more label here:
> 
>     out:
> 	gi2c->err = ret;
> 
> 	return ret;
> 
> in order to avoid always assigning twice
Thanks, added new label as out and handled it.
> 
>>   }
>>   
>> @@ -590,6 +697,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>   	unsigned long time_left;
>>   	dma_addr_t tx_addr, rx_addr;
>>   	void *tx_buf = NULL, *rx_buf = NULL;
>> +	struct gpi_multi_xfer *tx_multi_xfer;
>>   	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>>   
>>   	config.peripheral_config = &peripheral;
>> @@ -603,6 +711,39 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>   	peripheral.set_config = 1;
>>   	peripheral.multi_msg = false;
>>   
>> +	gi2c->gpi_config = &peripheral;
>> +	gi2c->num_msgs = num;
>> +	gi2c->is_tx_multi_xfer = false;
>> +	gi2c->tx_irq_cnt = 0;
>> +
>> +	tx_multi_xfer = &peripheral.multi_xfer;
>> +	tx_multi_xfer->msg_idx_cnt = 0;
>> +	tx_multi_xfer->buf_idx = 0;
>> +	tx_multi_xfer->unmap_msg_cnt = 0;
>> +	tx_multi_xfer->freed_msg_cnt = 0;
>> +	tx_multi_xfer->irq_msg_cnt = 0;
>> +	tx_multi_xfer->irq_cnt = 0;
> 
> you can initialize tx_multi_xfer to "{ };" to avoid all these
> " = 0"
Sure, done memset tx_multi_xfer to 0 in V2 patch.
> 
>> +
>> +	/*
>> +	 * If number of write messages are four and higher then
>> +	 * configure hardware for multi descriptor transfers with BEI.
>> +	 */
>> +	if (num >= MIN_NUM_OF_MSGS_MULTI_DESC) {
>> +		gi2c->is_tx_multi_xfer = true;
>> +		for (i = 0; i < num; i++) {
>> +			if (msgs[i].flags & I2C_M_RD) {
>> +				/*
>> +				 * Multi descriptor transfer with BEI
>> +				 * support is enabled for write transfers.
>> +				 * Add BEI optimization support for read
>> +				 * transfers later.
>> +				 */
>> +				gi2c->is_tx_multi_xfer = false;
>> +				break;
>> +			}
>> +		}
>> +	}
>> +
>>   	for (i = 0; i < num; i++) {
>>   		gi2c->cur = &msgs[i];
>>   		gi2c->err = 0;
>> @@ -613,14 +754,16 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>   			peripheral.stretch = 1;
>>   
>>   		peripheral.addr = msgs[i].addr;
>> +		if (i > 0 && (!(msgs[i].flags & I2C_M_RD)))
>> +			peripheral.multi_msg = false;
>>   
>> -		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
>> +		ret =  geni_i2c_gpi(gi2c, msgs, i, &config,
> 
> what is the point of passing 'i' if you always refer to msgs[i]
> in geni_i2c_gpi()?
Handled with new variable in "geni_i2c_gpi "and so no need to pass 
current i2c msg index, removed it in V2 patch.
> 
>>   				    &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
>>   		if (ret)
>>   			goto err;
>>   
>>   		if (msgs[i].flags & I2C_M_RD) {
>> -			ret =  geni_i2c_gpi(gi2c, &msgs[i], &config,
>> +			ret =  geni_i2c_gpi(gi2c, msgs, i, &config,
>>   					    &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
>>   			if (ret)
>>   				goto err;
>> @@ -628,18 +771,28 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
>>   			dma_async_issue_pending(gi2c->rx_c);
>>   		}
>>   
>> -		dma_async_issue_pending(gi2c->tx_c);
>> -
>> -		time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>> -		if (!time_left)
>> -			gi2c->err = -ETIMEDOUT;
>> +		if (!gi2c->is_tx_multi_xfer) {
>> +			dma_async_issue_pending(gi2c->tx_c);
>> +			time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
>> +			if (!time_left) {
>> +				dev_err(gi2c->se.dev, "%s:I2C timeout\n", __func__);
>> +				gi2c->err = -ETIMEDOUT;
>> +			}
>> +		}
>>   
>>   		if (gi2c->err) {
>>   			ret = gi2c->err;
>>   			goto err;
>>   		}
>>   
>> -		geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
>> +		if (!gi2c->is_tx_multi_xfer) {
>> +			geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
>> +		} else {
>> +			if (gi2c->tx_irq_cnt != tx_multi_xfer->irq_cnt) {
> 
>     else if (...) {
>     	...
>     }
Sure, else if used here in V2 patch.
> 
> Andi

Regards,
JyothiKumar