On Tue, May 06, 2025 at 04:48:44PM +0530, Jyothi Kumar Seerapu wrote:
> The I2C driver gets an interrupt upon transfer completion.
> When handling multiple messages in a single transfer, this
> results in N interrupts for N messages, leading to significant
> software interrupt latency.
>
> To mitigate this latency, utilize Block Event Interrupt (BEI)
> mechanism. Enabling BEI instructs the hardware to prevent interrupt
> generation and BEI is disabled when an interrupt is necessary.
>
> Large I2C transfer can be divided into chunks of 8 messages internally.
> Interrupts are not expected for the first 7 message completions, only
> the last message triggers an interrupt, indicating the completion of
> 8 messages. This BEI mechanism enhances overall transfer efficiency.
Why do you need this complexity? Is it possible to set the
DMA_PREP_INTERRUPT flag on the last message in the transfer?
>
> 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_jseer...@quicinc.com>
> ---
> v5 -> v6:
> - Instead of using bei_flag, moved the logic to use with DMA
> supported flags like DMA_PREP_INTERRUPT.
> - Additional parameter comments removed from geni_i2c_gpi_multi_desc_unmap
> function documentation.
>
> v4 -> v5:
> - Block event interrupt flag naming changed from flags to bei_flag.
> - Documentation added for "struct geni_i2c_dev".
>
> v3 -> v4:
> - API's added for Block event interrupt with multi descriptor support for
> I2C is moved from qcom-gpi-dma.h file to I2C geni qcom driver file.
> - gpi_multi_xfer_timeout_handler function is moved from GPI driver to
> I2C driver.
> - geni_i2c_gpi_multi_desc_xfer structure is added as a member of
> struct geni_i2c_dev.
>
> v2 -> v3:
> - In i2c_gpi_cb_result function, moved the logic of
> "!is_tx_multi_xfer" to else.
> - MIN_NUM_OF_MSGS_MULTI_DESC changed from 4 to 2
> - Updated commit description
>
> v1 -> v2:
> - Moved gi2c_gpi_xfer->msg_idx_cnt to separate local variable.
> - Updated goto labels for error scenarios in geni_i2c_gpi function
> - memset tx_multi_xfer to 0.
> - Removed passing current msg index to geni_i2c_gpi
> - Fixed kernel test robot reported compilation issues.
>
>
> drivers/i2c/busses/i2c-qcom-geni.c | 307 ++++++++++++++++++++++++++---
> 1 file changed, 280 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c
> b/drivers/i2c/busses/i2c-qcom-geni.c
> index 515a784c951c..e390cf5b4ddc 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -78,6 +78,62 @@ enum geni_i2c_err_code {
> #define XFER_TIMEOUT HZ
> #define RST_TIMEOUT HZ
>
> +#define QCOM_I2C_GPI_MAX_NUM_MSGS 16
> +#define QCOM_I2C_GPI_NUM_MSGS_PER_IRQ 8
> +#define QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC 2
> +
> +/**
> + * struct geni_i2c_gpi_multi_desc_xfer - Used for multi transfer support
> + *
> + * @msg_idx_cnt: message index for the transfer
> + * @buf_idx: dma buffer index
> + * @unmap_msg_cnt: unmapped transfer index
> + * @freed_msg_cnt: freed transfer index
> + * @irq_cnt: received interrupt count
> + * @irq_msg_cnt: transfer message count for the received irqs
> + * @dma_buf: virtual addresses of the buffers
> + * @dma_addr: dma addresses of the buffers
> + */
> +struct geni_i2c_gpi_multi_desc_xfer {
> + u32 msg_idx_cnt;
> + u32 buf_idx;
> + u32 unmap_msg_cnt;
> + u32 freed_msg_cnt;
> + u32 irq_cnt;
> + u32 irq_msg_cnt;
> + void *dma_buf[QCOM_I2C_GPI_MAX_NUM_MSGS];
> + dma_addr_t dma_addr[QCOM_I2C_GPI_MAX_NUM_MSGS];
> +};
> +
> +/**
> + * struct geni_i2c_dev - I2C Geni device specific structure
> + *
> + * @se: geni serial engine
> + * @tx_wm: Tx watermark level
> + * @irq: i2c serial engine interrupt
> + * @err: specifies error codes in i2c transfer failures
> + * @adap: i2c geni adapter
> + * @done: completion variable
> + * @cur: pointer to the i2c_msg mentioning current i2c message in use
> + * @cur_wr: variable used for i2c write opertions
> + * @cur_rd: variable used for i2c read operations
> + * @lock: spinlock variable used for synchronization
> + * @core_clk: pointer to clk
> + * @clk_freq_out: contains the i2c clock frequency
> + * @clk_fld: pointer to geni_i2c_clk_fld
> + * @suspended: flag used for system supend status
> + * @dma_buf: virtual address of the buffer
> + * @xfer_len: holds length for the dma operation
> + * @dma_addr: dma address of the buffer
> + * @tx_c: Tx dma channel
> + * @rx_c: Rx dma channel
> + * @gpi_mode: GPI DMA mode of operation
> + * @abort_done: true for marking i2c abort transfer
> + * @is_tx_multi_desc_xfer: true for i2c multi transfer support
> + * @num_msgs: number of i2c messages in a transfer
> + * @tx_irq_cnt: flag used for tx irq count in i2c multi transfer
> + * @i2c_multi_desc_config: used for multi transfer support
> + */
Unrelated. Documentation should go to a separate patch.
> struct geni_i2c_dev {
> struct geni_se se;
> u32 tx_wm;
> @@ -100,6 +156,10 @@ struct geni_i2c_dev {
> struct dma_chan *rx_c;
> bool gpi_mode;
> bool abort_done;
> + bool is_tx_multi_desc_xfer;
> + u32 num_msgs;
> + u32 tx_irq_cnt;
> + struct geni_i2c_gpi_multi_desc_xfer i2c_multi_desc_config;
> };
>
> struct geni_i2c_desc {
> @@ -500,6 +560,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 geni_i2c_gpi_multi_desc_xfer *tx_multi_xfer;
Define it in the corresponding chunk.
>
> if (result->result != DMA_TRANS_NOERROR) {
> dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
> @@ -508,7 +569,22 @@ 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_desc_xfer) {
> + complete(&gi2c->done);
> + } else {
> + tx_multi_xfer = &gi2c->i2c_multi_desc_config;
> +
> + /*
> + * Send Completion for last message or multiple of
> + * QCOM_I2C_GPI_NUM_MSGS_PER_IRQ.
> + */
> + if ((tx_multi_xfer->irq_msg_cnt == gi2c->num_msgs - 1) ||
> + (!((tx_multi_xfer->irq_msg_cnt + 1) %
> QCOM_I2C_GPI_NUM_MSGS_PER_IRQ))) {
> + tx_multi_xfer->irq_cnt++;
> + complete(&gi2c->done);
> + }
> + tx_multi_xfer->irq_msg_cnt++;
> + }
> }
>
> static void geni_i2c_gpi_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg
> *msg,
> @@ -526,38 +602,140 @@ 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,
> +/**
> + * geni_i2c_gpi_multi_desc_unmap() - unmaps the buffers post multi message
> TX transfers
> + * @gi2c: i2c dev handle
> + * @msgs: i2c messages array
> + * @peripheral: pointer to gpi_i2c_config
> + */
> +static void geni_i2c_gpi_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 geni_i2c_gpi_multi_desc_xfer *tx_multi_xfer =
> &gi2c->i2c_multi_desc_config;
> +
> + /*
> + * 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 *
> QCOM_I2C_GPI_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_I2C_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, (dma_addr_t)NULL);
> + tx_multi_xfer->freed_msg_cnt++;
> + }
> +}
> +
> +/**
> + * geni_i2c_gpi_multi_xfer_timeout_handler() - Handle multi message transfer
> timeout
> + * @dev: pointer to the corresponding dev node
> + * @multi_xfer: pointer to the geni_i2c_gpi_multi_desc_xfer
> + * @num_xfers: total number of transfers
> + * @transfer_timeout_msecs: transfer timeout value
> + * @transfer_comp: completion object of the transfer
> + *
> + * This function is used to wait for the processed transfers based on
> + * the interrupts generated upon transfer completion.
> + * Return: On success returns 0, otherwise return error code (-ETIMEDOUT)
> + */
> +static int geni_i2c_gpi_multi_xfer_timeout_handler(struct device *dev,
> + struct
> geni_i2c_gpi_multi_desc_xfer *multi_xfer,
> + u32 num_xfers, u32
> transfer_timeout_msecs,
> + struct completion
> *transfer_comp)
> +{
> + int i;
> + u32 max_irq_cnt, time_left;
> +
> + max_irq_cnt = num_xfers / QCOM_I2C_GPI_NUM_MSGS_PER_IRQ;
> + if (num_xfers % QCOM_I2C_GPI_NUM_MSGS_PER_IRQ)
> + max_irq_cnt++;
> +
> + /*
> + * Wait for the interrupts of the processed transfers in multiple
> + * of 8 and for the last transfer. If the hardware is fast and
> + * already processed all the transfers then no need to wait.
> + */
> + for (i = 0; i < max_irq_cnt; i++) {
> + reinit_completion(transfer_comp);
> + if (max_irq_cnt != multi_xfer->irq_cnt) {
> + time_left = wait_for_completion_timeout(transfer_comp,
> +
> transfer_timeout_msecs);
> + if (!time_left) {
> + dev_err(dev, "%s: Transfer timeout\n",
> __func__);
> + return -ETIMEDOUT;
> + }
> + }
> + if (num_xfers > multi_xfer->msg_idx_cnt)
> + return 0;
> + }
> + return 0;
> +}
> +
> +static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[],
> struct dma_slave_config *config, dma_addr_t *dma_addr_p,
> void **buf, unsigned int op, struct dma_chan *dma_chan)
> {
> struct gpi_i2c_config *peripheral;
> - unsigned int flags;
> void *dma_buf;
> dma_addr_t addr;
> enum dma_data_direction map_dirn;
> enum dma_transfer_direction dma_dirn;
> struct dma_async_tx_descriptor *desc;
> int ret;
> + struct geni_i2c_gpi_multi_desc_xfer *gi2c_gpi_xfer;
> + dma_cookie_t cookie;
> + u32 msg_idx;
>
> peripheral = config->peripheral_config;
> -
> - dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
> - if (!dma_buf)
> - return -ENOMEM;
> + gi2c_gpi_xfer = &gi2c->i2c_multi_desc_config;
> + dma_buf = gi2c_gpi_xfer->dma_buf[gi2c_gpi_xfer->buf_idx];
> + addr = gi2c_gpi_xfer->dma_addr[gi2c_gpi_xfer->buf_idx];
> + msg_idx = gi2c_gpi_xfer->msg_idx_cnt;
> +
> + dma_buf = i2c_get_dma_safe_msg_buf(&msgs[msg_idx], 1);
> + if (!dma_buf) {
> + ret = -ENOMEM;
> + goto out;
> + }
>
> 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[msg_idx].len, map_dirn);
> if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
> - i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
> - return -ENOMEM;
> + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[msg_idx], false);
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + if (gi2c->is_tx_multi_desc_xfer) {
> + if (((msg_idx + 1) % QCOM_I2C_GPI_NUM_MSGS_PER_IRQ))
> + peripheral->dma_flags = DMA_CTRL_ACK;
> + else
> + peripheral->dma_flags = DMA_PREP_INTERRUPT |
> DMA_CTRL_ACK;
> +
> + /* BEI bit to be cleared for last TRE */
> + if (msg_idx == gi2c->num_msgs - 1)
> + peripheral->dma_flags = DMA_PREP_INTERRUPT |
> DMA_CTRL_ACK;
> + } else {
> + peripheral->dma_flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
No-no-no. There is no need to set peripheral->dma_flags.
> }
>
> /* set the length as message for rx txn */
> - peripheral->rx_len = msg->len;
> + peripheral->rx_len = msgs[msg_idx].len;
> peripheral->op = op;
>
> ret = dmaengine_slave_config(dma_chan, config);
> @@ -568,14 +746,14 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c,
> struct i2c_msg *msg,
>
> peripheral->set_config = 0;
> peripheral->multi_msg = true;
> - flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
>
> if (op == I2C_WRITE)
> dma_dirn = DMA_MEM_TO_DEV;
> 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[msg_idx].len,
> + dma_dirn, peripheral->dma_flags);
> if (!desc) {
> dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
> ret = -EIO;
> @@ -585,15 +763,48 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c,
> struct i2c_msg *msg,
> desc->callback_result = i2c_gpi_cb_result;
> desc->callback_param = gi2c;
>
> - dmaengine_submit(desc);
> - *buf = dma_buf;
> - *dma_addr_p = addr;
> + if (!((msgs[msg_idx].flags & I2C_M_RD) && op == I2C_WRITE)) {
> + gi2c_gpi_xfer->msg_idx_cnt++;
> + gi2c_gpi_xfer->buf_idx = (msg_idx + 1) %
> QCOM_I2C_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);
> + ret = -EINVAL;
> + goto err_config;
> + }
>
> + if (gi2c->is_tx_multi_desc_xfer) {
> + dma_async_issue_pending(gi2c->tx_c);
> + if ((msg_idx == (gi2c->num_msgs - 1)) ||
> + (gi2c_gpi_xfer->msg_idx_cnt >=
> + QCOM_I2C_GPI_MAX_NUM_MSGS + gi2c_gpi_xfer->freed_msg_cnt))
> {
> + ret =
> geni_i2c_gpi_multi_xfer_timeout_handler(gi2c->se.dev, gi2c_gpi_xfer,
> +
> gi2c->num_msgs, XFER_TIMEOUT,
> +
> &gi2c->done);
> + if (ret) {
> + dev_err(gi2c->se.dev,
> + "I2C multi write msg transfer timeout:
> %d\n",
> + ret);
> + 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[msg_idx].len, map_dirn);
> + i2c_put_dma_safe_msg_buf(dma_buf, &msgs[msg_idx], false);
> +
> +out:
> + gi2c->err = ret;
> return ret;
> }
>
> @@ -605,6 +816,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 geni_i2c_gpi_multi_desc_xfer *tx_multi_xfer;
> const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>
> config.peripheral_config = &peripheral;
> @@ -618,6 +830,33 @@ 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->num_msgs = num;
> + gi2c->is_tx_multi_desc_xfer = false;
> + gi2c->tx_irq_cnt = 0;
> +
> + tx_multi_xfer = &gi2c->i2c_multi_desc_config;
> + memset(tx_multi_xfer, 0, sizeof(struct geni_i2c_gpi_multi_desc_xfer));
> +
> + /*
> + * If number of write messages are two and higher then
> + * configure hardware for multi descriptor transfers with BEI.
> + */
> + if (num >= QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC) {
> + gi2c->is_tx_multi_desc_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.
> + * TODO: Add BEI optimization support for
> + * read transfers later.
> + */
> + gi2c->is_tx_multi_desc_xfer = false;
> + break;
> + }
> + }
> + }
> +
> for (i = 0; i < num; i++) {
> gi2c->cur = &msgs[i];
> gi2c->err = 0;
> @@ -628,14 +867,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, &config,
> &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
Can't you just pass the flag to set DMA_PREP_INTERRUPT for the last
message here and on the next geni_i2c_gpi() call?
> 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, &config,
> &rx_addr, &rx_buf, I2C_READ,
> gi2c->rx_c);
> if (ret)
> goto err;
> @@ -643,18 +884,26 @@ 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_desc_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_desc_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;
> + geni_i2c_gpi_multi_desc_unmap(gi2c, msgs, &peripheral);
> + }
> }
>
> return num;
> @@ -663,7 +912,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_desc_xfer)
> + geni_i2c_gpi_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
>
--
With best wishes
Dmitry
