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

[Jyothi Kumar Seerapu <jyothi.seerapu@oss.qualcomm.com>]

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)
>   	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)
>