In GPI mode, the I2C GENI driver incorrectly generates an extra TX DMA TRE on the TX channel during single read message transfer. This results in an unnecessary write operation on the I2C bus, which is not required.
Update the logic to avoid generating the extra TX DMA TRE for single read message, ensuring correct behavior and preventing redundant transfers.
Co-developed-by: Maramaina Naresh naresh.maramaina@oss.qualcomm.com Signed-off-by: Maramaina Naresh naresh.maramaina@oss.qualcomm.com Signed-off-by: Aniket Randive aniket.randive@oss.qualcomm.com --- drivers/i2c/busses/i2c-qcom-geni.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index a4acb78fafb6..2706309bbebb 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -625,8 +625,8 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], { struct gpi_i2c_config *peripheral; unsigned int flags; - void *dma_buf; - dma_addr_t addr; + void *dma_buf = NULL; + dma_addr_t addr = 0; enum dma_data_direction map_dirn; enum dma_transfer_direction dma_dirn; struct dma_async_tx_descriptor *desc; @@ -639,6 +639,11 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], gi2c_gpi_xfer = &gi2c->i2c_multi_desc_config; msg_idx = gi2c_gpi_xfer->msg_idx_cnt;
+ if (op == I2C_WRITE && msgs[msg_idx].flags & I2C_M_RD) { + peripheral->multi_msg = true; + goto skip_dma; + } + dma_buf = i2c_get_dma_safe_msg_buf(&msgs[msg_idx], 1); if (!dma_buf) { ret = -ENOMEM; @@ -668,6 +673,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; }
+skip_dma: /* set the length as message for rx txn */ peripheral->rx_len = msgs[msg_idx].len; peripheral->op = op; @@ -740,9 +746,11 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], return 0;
err_config: - 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); + if (op == I2C_WRITE && (msgs[msg_idx].flags & I2C_M_RD)) { + 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;
--- base-commit: 785f0eb2f85decbe7c1ef9ae922931f0194ffc2e change-id: 20260325-skip_extra_dma_tre-a3cf22f81d9b
Best regards, -- Aniket Randive aniket.randive@oss.qualcomm.com
On 3/26/2026 10:01 AM, Aniket Randive wrote:
In GPI mode, the I2C GENI driver incorrectly generates an extra TX DMA TRE on the TX channel during single read message transfer. This results
What's the impact of this extra DMA TRE ? do you see failure/timeout, anything ?
in an unnecessary write operation on the I2C bus, which is not required.
Update the logic to avoid generating the extra TX DMA TRE for single read message, ensuring correct behavior and preventing redundant transfers.
So for read, we do unwanted write too ? if so, please write it accordingly. Correct behavior needs to be justified against wrong.
Co-developed-by: Maramaina Naresh naresh.maramaina@oss.qualcomm.com Signed-off-by: Maramaina Naresh naresh.maramaina@oss.qualcomm.com Signed-off-by: Aniket Randive aniket.randive@oss.qualcomm.com
drivers/i2c/busses/i2c-qcom-geni.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c index a4acb78fafb6..2706309bbebb 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -625,8 +625,8 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], { struct gpi_i2c_config *peripheral; unsigned int flags;
- void *dma_buf;
- dma_addr_t addr;
- void *dma_buf = NULL;
- dma_addr_t addr = 0; enum dma_data_direction map_dirn; enum dma_transfer_direction dma_dirn; struct dma_async_tx_descriptor *desc;
@@ -639,6 +639,11 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], gi2c_gpi_xfer = &gi2c->i2c_multi_desc_config; msg_idx = gi2c_gpi_xfer->msg_idx_cnt;
- if (op == I2C_WRITE && msgs[msg_idx].flags & I2C_M_RD) {
peripheral->multi_msg = true;
what's the actual meaning of multi_msg here ? IIUC, this multi_msg is set to true for single transfer ? any better name if so ? Yes, need to change it out of this patch.
goto skip_dma;- }
- dma_buf = i2c_get_dma_safe_msg_buf(&msgs[msg_idx], 1); if (!dma_buf) { ret = -ENOMEM;
@@ -668,6 +673,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; } +skip_dma: /* set the length as message for rx txn */ peripheral->rx_len = msgs[msg_idx].len; peripheral->op = op; @@ -740,9 +746,11 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], return 0; err_config:
- 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);
- if (op == I2C_WRITE && (msgs[msg_idx].flags & I2C_M_RD)) {
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;
base-commit: 785f0eb2f85decbe7c1ef9ae922931f0194ffc2e change-id: 20260325-skip_extra_dma_tre-a3cf22f81d9b
Best regards,
Aniket Randive aniket.randive@oss.qualcomm.com
On 3/27/2026 11:51 AM, Mukesh Kumar Savaliya wrote:
On 3/26/2026 10:01 AM, Aniket Randive wrote:
In GPI mode, the I2C GENI driver incorrectly generates an extra TX DMA TRE on the TX channel during single read message transfer. This results
What's the impact of this extra DMA TRE ? do you see failure/timeout, anything ?
This write operation is unnecessary. For a 1‑byte read operation, only the CONFIG, GO and RX DMA TRE are required. However, an additional TX DMA TRE is currently being added. In addition to being redundant, this also results in unnecessary DMA buffer mapping for the TX DMA TRE.
in an unnecessary write operation on the I2C bus, which is not required.
Update the logic to avoid generating the extra TX DMA TRE for single read message, ensuring correct behavior and preventing redundant transfers.
So for read, we do unwanted write too ? if so, please write it accordingly. Correct behavior needs to be justified against wrong.
Yes. Currently, the driver performs an unnecessary write as part of a read transaction. For a single‑byte read operation, the correct behavior is to issue only the CONFIG, GO command, and an RX DMA TRE. This TX DMA TRE does not contribute to the read operation and results in an unintended write and redundant DMA buffer mapping. Hence, the current behavior is incorrect and should be fixed to align with the required hardware transaction sequence.
Co-developed-by: Maramaina Naresh naresh.maramaina@oss.qualcomm.com Signed-off-by: Maramaina Naresh naresh.maramaina@oss.qualcomm.com Signed-off-by: Aniket Randive aniket.randive@oss.qualcomm.com
drivers/i2c/busses/i2c-qcom-geni.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/ i2c-qcom-geni.c index a4acb78fafb6..2706309bbebb 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -625,8 +625,8 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], { struct gpi_i2c_config *peripheral; unsigned int flags; - void *dma_buf; - dma_addr_t addr; + void *dma_buf = NULL; + dma_addr_t addr = 0; enum dma_data_direction map_dirn; enum dma_transfer_direction dma_dirn; struct dma_async_tx_descriptor *desc; @@ -639,6 +639,11 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], gi2c_gpi_xfer = &gi2c->i2c_multi_desc_config; msg_idx = gi2c_gpi_xfer->msg_idx_cnt; + if (op == I2C_WRITE && msgs[msg_idx].flags & I2C_M_RD) { + peripheral->multi_msg = true;
what's the actual meaning of multi_msg here ? IIUC, this multi_msg is set to true for single transfer ? any better name if so ? Yes, need to change it out of this patch.
In the GPI driver, a DMA TRE is created only when either the operation is a read or when multi_msg is set to false. This is controlled by the following check during I2C TRE construction,
if (i2c->op == I2C_READ || i2c->multi_msg == false) { /* create the DMA TRE */ tre = &desc->tre[tre_idx];
Previously, when dmaengine_prep_slave_single() was invoked for a write operation, this condition evaluated to true and a TX DMA TRE was created on the TX channel. With the recent change, the flag is explicitly set, which correctly prevents creation of a TX DMA TRE. I agree the variable name can be improved for clarity and propose addressing that in a follow‑up patch to keep this change minimal and focused.
+ goto skip_dma; + }
dma_buf = i2c_get_dma_safe_msg_buf(&msgs[msg_idx], 1); if (!dma_buf) { ret = -ENOMEM; @@ -668,6 +673,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; } +skip_dma: /* set the length as message for rx txn */ peripheral->rx_len = msgs[msg_idx].len; peripheral->op = op; @@ -740,9 +746,11 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], return 0; err_config: - 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); + if (op == I2C_WRITE && (msgs[msg_idx].flags & I2C_M_RD)) { + 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;
base-commit: 785f0eb2f85decbe7c1ef9ae922931f0194ffc2e change-id: 20260325-skip_extra_dma_tre-a3cf22f81d9b
Best regards,
Aniket Randive aniket.randive@oss.qualcomm.com
On 3/27/2026 3:37 PM, Aniket RANDIVE wrote:
On 3/27/2026 11:51 AM, Mukesh Kumar Savaliya wrote:
On 3/26/2026 10:01 AM, Aniket Randive wrote:
In GPI mode, the I2C GENI driver incorrectly generates an extra TX DMA TRE on the TX channel during single read message transfer. This results
What's the impact of this extra DMA TRE ? do you see failure/timeout, anything ?
This write operation is unnecessary. For a 1‑byte read operation, only the CONFIG, GO and RX DMA TRE are required. However, an additional TX DMA TRE is currently being added. In addition to being redundant, this also results in unnecessary DMA buffer mapping for the TX DMA TRE.
Ok, makes sense.
in an unnecessary write operation on the I2C bus, which is not required.
Update the logic to avoid generating the extra TX DMA TRE for single read message, ensuring correct behavior and preventing redundant transfers.
So for read, we do unwanted write too ? if so, please write it accordingly. Correct behavior needs to be justified against wrong.
Yes. Currently, the driver performs an unnecessary write as part of a read transaction. For a single‑byte read operation, the correct behavior is to issue only the CONFIG, GO command, and an RX DMA TRE. This TX DMA TRE does not contribute to the read operation and results in an unintended write and redundant DMA buffer mapping. Hence, the current behavior is incorrect and should be fixed to align with the required hardware transaction sequence.
Makes sense, please add such details in the commit log of next patch to make it clear against the problem and solution. Rest of the changes looks good.
Co-developed-by: Maramaina Naresh naresh.maramaina@oss.qualcomm.com Signed-off-by: Maramaina Naresh naresh.maramaina@oss.qualcomm.com Signed-off-by: Aniket Randive aniket.randive@oss.qualcomm.com
drivers/i2c/busses/i2c-qcom-geni.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/ i2c-qcom-geni.c index a4acb78fafb6..2706309bbebb 100644 --- a/drivers/i2c/busses/i2c-qcom-geni.c +++ b/drivers/i2c/busses/i2c-qcom-geni.c @@ -625,8 +625,8 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], { struct gpi_i2c_config *peripheral; unsigned int flags; - void *dma_buf; - dma_addr_t addr; + void *dma_buf = NULL; + dma_addr_t addr = 0; enum dma_data_direction map_dirn; enum dma_transfer_direction dma_dirn; struct dma_async_tx_descriptor *desc; @@ -639,6 +639,11 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], gi2c_gpi_xfer = &gi2c->i2c_multi_desc_config; msg_idx = gi2c_gpi_xfer->msg_idx_cnt; + if (op == I2C_WRITE && msgs[msg_idx].flags & I2C_M_RD) { + peripheral->multi_msg = true;
what's the actual meaning of multi_msg here ? IIUC, this multi_msg is set to true for single transfer ? any better name if so ? Yes, need to change it out of this patch.
In the GPI driver, a DMA TRE is created only when either the operation is a read or when multi_msg is set to false. This is controlled by the following check during I2C TRE construction,
if (i2c->op == I2C_READ || i2c->multi_msg == false) { /* create the DMA TRE */ tre = &desc->tre[tre_idx];
Previously, when dmaengine_prep_slave_single() was invoked for a write operation, this condition evaluated to true and a TX DMA TRE was created on the TX channel. With the recent change, the flag is explicitly set, which correctly prevents creation of a TX DMA TRE. I agree the variable name can be improved for clarity and propose addressing that in a follow‑up patch to keep this change minimal and focused.
Right, that was my intent too. I think the variable name is misleading.
+ goto skip_dma; + }
dma_buf = i2c_get_dma_safe_msg_buf(&msgs[msg_idx], 1); if (!dma_buf) { ret = -ENOMEM; @@ -668,6 +673,7 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK; } +skip_dma: /* set the length as message for rx txn */ peripheral->rx_len = msgs[msg_idx].len; peripheral->op = op; @@ -740,9 +746,11 @@ static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], return 0; err_config: - 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); + if (op == I2C_WRITE && (msgs[msg_idx].flags & I2C_M_RD)) { + 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;
base-commit: 785f0eb2f85decbe7c1ef9ae922931f0194ffc2e change-id: 20260325-skip_extra_dma_tre-a3cf22f81d9b
Best regards,
Aniket Randive aniket.randive@oss.qualcomm.com
linaro-mm-sig@lists.linaro.org
-
Aniket RANDIVE -
Aniket Randive
-
Mukesh Kumar Savaliya