Re: [PATCH v4 5/5] i2c: qcom-cci: Enforce the required CCI clock rate

On 8/1/26 10:10 PM, Loic Poulain wrote:
> The CCI hw_params timing values are only valid at the specific clock
> rate they were calibrated for. A previous change made the driver select
> the timing set matching the currently running clock rate, but the rate
> itself was still left to the DT (assigned-clock-rates) or the bootloader,
> which is fragile: if no rate is enforced the timings may not match and
> violate the I2C specification.

[...]

> +/*
> + * The single CCI clock is shared by all masters, which may run in different
> + * modes. Pick the lowest rate that has a valid timing set for every active
> + * master's mode.> + */
> +static unsigned long cci_get_required_rate(struct cci *cci)
> +{
> +	int ri, i;

Please declare the loop iterators in the loop 'header', it's been
OKd inside the kernel for a while now

> +
> +	for (ri = 0; ri < NUM_CCI_CLK_RATES; ri++) {
> +		bool supported = true;
> +
> +		for (i = 0; i < cci->data->num_masters; i++) {
> +			int mode = cci->master[i].mode;
> +
> +			if (!cci->master[i].cci)
> +				continue;
> +
> +			if (mode > cci->data->max_mode ||
> +			    !cci_hw_params[ri][mode].thigh) {
> +				supported = false;
> +				break;
> +			}

This still goes to dev_pm_opp_set_rate() with a value of 0, which is
BAD - let's check the retval of this function and pass the rate via a
pointer parameter

> +		}
> +
> +		if (supported)
> +			return cci_clk_rates[ri];
> +	}
> +
> +	return 0;
> +}
> +
> +static int cci_set_core_rate(struct cci *cci, unsigned long rate)
> +{
> +	struct device *dev = cci->dev;
> +	int ret;
> +
> +	ret = dev_pm_opp_set_rate(dev, rate);
> +	if (ret) {
> +		dev_warn(dev, "CCI clock could not be set to %lu Hz\n", rate);
> +		return ret;
> +	}
> +
> +	if (!rate)
> +		return 0;
> +
> +	/*
> +	 * Sanity: The hw_params timings are only valid at the exact
> +	 * expected rate, verify what landed on the hardware.
> +	 */
> +	if (clk_get_rate(cci->cci_clk) != rate)
> +		dev_warn(dev, "CCI clock is not at expected %lu Hz\n", rate);
> +
> +	return 0;
> +}
> +
>  static int __maybe_unused cci_suspend_runtime(struct device *dev)
>  {
>  	struct cci *cci = dev_get_drvdata(dev);
>  
> +	cci_set_core_rate(cci, 0);

No, that's a footgun

https://lore.kernel.org/linux-arm-msm/[email protected]/

>  	cci_disable_clocks(cci);
>  	return 0;
>  }
> @@ -588,6 +646,10 @@ static int __maybe_unused cci_resume_runtime(struct device *dev)
>  	struct cci *cci = dev_get_drvdata(dev);
>  	int ret;
>  
> +	ret = cci_set_core_rate(cci, cci_get_required_rate(cci));
> +	if (ret)
> +		return ret;

You generally only need to set_rate once and then enable/disable the clocks,
so this can be removed

Konrad