Re: [PATCH v2 2/3] i2c: qcom-geni: Add bus recovery support for FIFO mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] i2c: qcom-geni: missing set_rate() call in geni_i2c_recover_bus()
- [Low] i2c: qcom-geni: undeclared XFER_TIMEOUT in geni_i2c_fifo_bus_cmd()
--

commit fa2e06c81faf514701416b8f6399729cc8d72319
Author: Aniket Randive <aniket.randive@oss.qualcomm.com>

i2c: qcom-geni: Add bus recovery support for FIFO mode

This commit adds I2C bus recovery support for FIFO mode using the
I2C_BUS_CLEAR and I2C_STOP_ON_BUS hardware opcodes. It triggers recovery
automatically from geni_i2c_xfer() on -EPROTO, -ETIMEDOUT, and -EAGAIN
errors, and registers geni_i2c_recover_bus() via i2c_bus_recovery_info.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 60b6273f538c2..065d7240de04d 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -1013,6 +1015,107 @@ static int geni_i2c_fifo_xfer(struct geni_i2c_dev *gi2c,
>  	return num;
>  }
>  
> +static int geni_i2c_fifo_bus_cmd(struct geni_i2c_dev *gi2c, u32 cmd)
> +{
> +	unsigned long time_left;
> +	unsigned long flags;
> +
> +	/*
> +	 * Clear cur so the IRQ handler does not attempt FIFO watermark
> +	 * filling or draining while the recovery opcode is in flight.
> +	 * cur and err are shared with geni_i2c_irq(), which reads cur and
> +	 * writes err under gi2c->lock, so take the lock around this reset.
> +	 */
> +	spin_lock_irqsave(&gi2c->lock, flags);
> +	gi2c->cur = NULL;
> +	gi2c->err = 0;
> +	spin_unlock_irqrestore(&gi2c->lock, flags);
> +	geni_se_select_mode(&gi2c->se, GENI_SE_FIFO);
> +	reinit_completion(&gi2c->done);
> +
> +	geni_se_setup_m_cmd(&gi2c->se, cmd, 0);
> +	time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);

[Severity: Low]
Will this break compilation at this commit due to XFER_TIMEOUT being
undeclared?

Commit 6b4917500125 ("i2c: qcom-geni: Add dynamic transfer timeout based on
transfer length and frequency") removed the XFER_TIMEOUT macro from
drivers/i2c/busses/i2c-qcom-geni.c in favor of gi2c->adap.timeout.

A subsequent commit in the series, 0f096af4f327 ("i2c: qcom-geni: Add bus
recovery support for GPI DMA mode"), updates this call in
geni_i2c_fifo_bus_cmd() to use gi2c->adap.timeout, so this commit fails to
build on its own and breaks bisection.

> +	if (!time_left) {
> +		dev_err(gi2c->se.dev, "timeout waiting for bus cmd %u\n", cmd);
> +		geni_i2c_abort_xfer(gi2c);
> +
> +		return -ETIMEDOUT;
> +	}

[ ... ]

> +static int geni_i2c_recover_bus(struct i2c_adapter *adap)
> +{
> +	struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
> +	u32 ios;
> +	int ret;
> +
> +	ret = pm_runtime_get_sync(gi2c->se.dev);
> +	if (ret < 0) {
> +		dev_err(gi2c->se.dev, "bus recovery failed, error turning SE resources:%d\n", ret);
> +		pm_runtime_put_noidle(gi2c->se.dev);
> +		return ret;
> +	}
> +
> +	if (geni_i2c_bus_is_free(gi2c)) {
> +		pm_runtime_put_autosuspend(gi2c->se.dev);
> +		return 0;
> +	}
> +
> +	/* Bus clear cannot recover a target holding SCL low. */
> +	ios = readl_relaxed(gi2c->se.base + SE_GENI_IOS);
> +	if (!(ios & IO2_DATA_IN)) {
> +		pm_runtime_put_autosuspend(gi2c->se.dev);
> +		return -EBUSY;
> +	}
> +
> +	ret = geni_i2c_fifo_bus_cmd(gi2c, I2C_BUS_CLEAR);

[Severity: Medium]
Should geni_i2c_recover_bus() call gi2c->dev_data->set_rate(&gi2c->se,
gi2c->clk_freq_out) after pm_runtime_get_sync() before issuing I2C_BUS_CLEAR
and I2C_STOP_ON_BUS?

In drivers/i2c/busses/i2c-qcom-geni.c, geni_i2c_xfer() is the only place that
invokes dev_data->set_rate():

geni_i2c_xfer()
  pm_runtime_resume_and_get(gi2c->se.dev)
  gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out)

For standard GENI I2C controllers, qcom_geni_i2c_conf() programs
SE_GENI_CLK_SEL, enables SER_CLK_EN with clk_div in GENI_SER_M_CLK_CFG, and
sets the SCL timing counters in SE_I2C_SCL_COUNTERS. For SA8255P,
geni_se_set_perf_opp() configures the performance domain OPP, which
geni_i2c_runtime_suspend() drops to 0 via geni_se_resources_deactivate().

If a caller invokes i2c_recover_bus() directly via adap->bus_recovery_info
before the first transfer after probe, or after runtime suspend has
deactivated the SE resources, wouldn't the recovery commands run without the
serial clock and SCL counters or OPP configured?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-20260826_aniket_randive_oss_qualcomm_com-v2-0-93572b453d6f@oss.qualcomm.com?part=2