drivers/i2c/busses/i2c-qcom-geni.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)
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>
>>>
>>>
>>
>
© 2016 - 2026 Red Hat, Inc.