Re: [PATCH v1 3/3] i2c: qcom-geni: Add bus recovery support for GPI DMA mode

[Aniket RANDIVE <aniket.randive@oss.qualcomm.com>]

On 9/3/2026 9:44 PM, Jyothi Kumar Seerapu wrote:
> 
> 
> On 8/26/2026 3:21 PM, Aniket Randive wrote:
>> I2C transfers in GPI DMA mode can fail with -EIO when a target holds
>> SDA low, leaving the bus stuck and preventing subsequent transactions.
>> The GPI completion callback reports a NACK and a genuine bus error
>> alike as -EIO, and the existing recovery path switches the SE to FIFO
>> mode which is not valid in GPI DMA mode.
>>
>> Add bus recovery support using the I2C_BUS_CLEAR and I2C_STOP_ON_BUS
>> opcodes issued via the GPI TX DMA channel. Guard the -EIO recovery
>> trigger with a SE_GENI_IOS RX_DATA_IN check to skip recovery when SDA
>> is already released. Extend the recovery callback to dispatch through
>> the GPI DMA path when gi2c->gpi_mode is set.
>>
>> Signed-off-by: Aniket Randive <aniket.randive@oss.qualcomm.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 104 +++++++++++++++++++++++++++ 
>> ++++------
>>   1 file changed, 88 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/ 
>> i2c-qcom-geni.c
>> index 9fa1a8ac400c..85cb1367958c 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -34,8 +34,6 @@
>>   #define I2C_READ        0x2
>>   #define I2C_WRITE_READ        0x3
>>   #define I2C_ADDR_ONLY        0x4
>> -#define I2C_BUS_CLEAR        0x6
>> -#define I2C_STOP_ON_BUS        0x7
>>   /* M_CMD params for I2C */
>>   #define PRE_CMD_DELAY        BIT(0)
>>   #define TIMESTAMP_BEFORE    BIT(1)
>> @@ -1003,6 +1001,70 @@ static int geni_i2c_fifo_bus_cmd(struct 
>> geni_i2c_dev *gi2c, u32 cmd)
>>       return gi2c->err;
>>   }
>> +static int geni_i2c_gpi_bus_cmd(struct geni_i2c_dev *gi2c, enum 
>> i2c_op cmd)
>> +{
>> +    const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>> +    struct dma_async_tx_descriptor *desc;
>> +    struct gpi_i2c_config peripheral = {};
>> +    struct dma_slave_config config = {};
>> +    unsigned long time_left;
>> +    unsigned long flags;
>> +    dma_cookie_t cookie;
>> +
>> +    config.peripheral_config = &peripheral;
>> +    config.peripheral_size = sizeof(peripheral);
>> +
>> +    peripheral.set_config  = 1;
>> +    peripheral.pack_enable = I2C_PACK_TX | I2C_PACK_RX;
>> +    peripheral.cycle_count = itr->t_cycle_cnt;
>> +    peripheral.high_count  = itr->t_high_cnt;
>> +    peripheral.low_count   = itr->t_low_cnt;
>> +    peripheral.clk_div     = itr->clk_div;
>> +    peripheral.op          = cmd;
>> +
>> +    if (dmaengine_slave_config(gi2c->tx_c, &config)) {
>> +        dev_err(gi2c->se.dev, "dma config error for bus cmd %u\n", cmd);
>> +        return -EIO;
>> +    }
>> +
>> +    desc = dmaengine_prep_slave_single(gi2c->tx_c, 0, 0, DMA_MEM_TO_DEV,
>> +                       DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> +    if (!desc)
>> +        return -EIO;
>> +
>> +    desc->callback_result = i2c_gpi_cb_result;
>> +    desc->callback_param = gi2c;
>> +
>> +    cookie = dmaengine_submit(desc);
>> +    if (dma_submit_error(cookie))
>> +        return -EIO;
>> +
>> +    /*
>> +     * cur and err are shared with geni_i2c_irq() and the GPI callback
>> +     * i2c_gpi_cb_result(); both write err (the IRQ handler under
>> +     * gi2c->lock). Reset them under the lock before issuing the 
>> transfer.
>> +     */
>> +    spin_lock_irqsave(&gi2c->lock, flags);
>> +    gi2c->cur = NULL;
>> +    gi2c->err = 0;
>> +    spin_unlock_irqrestore(&gi2c->lock, flags);
>> +    reinit_completion(&gi2c->done);
>> +    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, "timeout waiting for GPI bus cmd %u\n", 
>> cmd);
>> +        dmaengine_terminate_sync(gi2c->tx_c);
>> +        return -ETIMEDOUT;
>> +    }
>> +
>> +    /* ARB_LOST and BUS_PROTO may be expected during recovery; treat 
>> as success */
>> +    if (gi2c->err == -EAGAIN || gi2c->err == -EPROTO)
>> +        return 0;
>> +
>> +    return gi2c->err;
>> +}
>> +
>>   static int geni_i2c_recover_bus(struct i2c_adapter *adap)
>>   {
>>       struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
>> @@ -1021,15 +1083,23 @@ static int geni_i2c_recover_bus(struct 
>> i2c_adapter *adap)
>>           return 0;
>>       }
>> -    ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
>> -    if (!ret)
>> -        ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
>> +    if (gi2c->gpi_mode) {
>> +        ret = geni_i2c_gpi_bus_cmd(gi2c, I2C_BUS_CLEAR);
>> +        if (ret)
>> +            dev_dbg(gi2c->se.dev, "GPI bus clear returned %d, issuing 
>> stop anyway\n",
>> +                ret);
>> +
>> +        ret = geni_i2c_gpi_bus_cmd(gi2c, I2C_STOP_ON_BUS);
>> +    } else {
>> +        ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);
>> +        if (!ret)
>> +            ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_STOP_ON_BUS);
>> +    }
>>       /*
>> -     * Recovery succeeds only once the slave releases SDA, so the bus
>> -     * state is the authority: RX_DATA_IN high means the bus is free,
>> -     * while a clean opcode status with SDA still low is a failed
>> -     * recovery.
>> +     * Recovery succeeds when SDA is released. RX_DATA_IN high means the
>> +     * bus is free; trust this over the opcode return path, which cannot
>> +     * flag a benign recovery event in GPI mode.
>>        */
>>       if (readl_relaxed(gi2c->se.base + SE_GENI_IOS) & RX_DATA_IN)
>>           ret = 0;
>> @@ -1066,14 +1136,18 @@ static int geni_i2c_xfer(struct i2c_adapter 
>> *adap,
>>       else
>>           ret = geni_i2c_fifo_xfer(gi2c, msgs, num);
>> -    if (!gi2c->gpi_mode &&
>> -        (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN)) {
>> +    if (ret == -EPROTO || ret == -ETIMEDOUT || ret == -EAGAIN ||
>> +        (gi2c->gpi_mode && ret == -EIO)) {
>>           /*
>>            * Only attempt recovery if SDA is stuck low. -EPROTO and
>>            * -ETIMEDOUT indicate bus errors where the target may be
>>            * holding SDA low. ARB_LOST (-EAGAIN) on a single-controller
>>            * bus indicates a stuck target, not a real arbitration loss.
>> -         * GPI DMA mode extends this trigger separately.
>> +         * In GPI DMA mode the completion callback reports NACK,
>> +         * BUS_PROTO and ARB_LOST alike as -EIO, so the SE_GENI_IOS
>> +         * RX_DATA_IN check below is what distinguishes a genuinely
>> +         * stuck bus from a benign NACK: a NACK leaves SDA released
>> +         * (high) after the STOP, so recovery is skipped.
>>            */
>>           if (!(readl_relaxed(gi2c->se.base + SE_GENI_IOS) & 
>> RX_DATA_IN)) {
>>               int recovery_ret = i2c_recover_bus(adap);
> This does not appear to handle scenarios such as an I2C multi-message 
> transfer where the stretch bit is set during the first message, but an 
> error occurs while preparing the second message (for example, during 
> memory allocation or DMA descriptor preparation). In such cases, the SCL 
> line may remain asserted low and impact subsequent transfers.
>   > @@ -1292,10 +1366,8 @@ static int geni_i2c_probe(struct 
> platform_device *pdev)

Thank you for pointing this out.
The GPI completion callback reports transfer failures as -EIO, but 
failures while preparing a later descriptor may return other errors such 
as -ENOMEM or -EINVAL. If a preceding stretched transfer has left SCL 
low, this requires separate consideration.
However, the bus-clear sequence is only applicable when SCL is high and 
SDA is held low, since it recovers the bus by generating SCL pulses. It 
cannot recover a bus where SCL itself is held low. The recovery callback 
therefore detects SCL-low and returns -EBUSY without issuing 
I2C_BUS_CLEAR or I2C_STOP_ON_BUS.
The GPI error path already calls dmaengine_terminate_sync() for both 
channels. I will verify whether this releases a controller-owned stretch 
before extending the recovery trigger to descriptor-preparation 
failures. If termination does not release SCL, the appropriate fix is a 
GPI/GENI abort or reset path, rather than invoking bus-clear.

- Aniket

>>       if (ret < 0)
>>           return ret;
>> -    if (!gi2c->gpi_mode) {
>> -        gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
>> -        gi2c->adap.bus_recovery_info = &gi2c->rinfo;
>> -    }
>> +    gi2c->rinfo.recover_bus = geni_i2c_recover_bus;
>> +    gi2c->adap.bus_recovery_info = &gi2c->rinfo;
>>       ret = i2c_add_adapter(&gi2c->adap);
>>       if (ret)
>>
>