Re: [PATCH v4 2/5] i2c: qcom-cci: Support per-mode CCI clock rates

On 8/1/26 23:10, Loic Poulain wrote:
> The CCI hw_params timing values (thigh, tlow, etc.) are expressed in
> clock ticks and are only valid at the specific CCI clock rate they were
> calibrated for. Different I2C modes may be calibrated for different
> rates, and the single CCI clock is shared by all masters.
> 
> Turn the timing table into a two-dimensional [rate][mode] matrix so a
> given rate can carry timing sets for each mode, and select the entry
> matching the currently running clock rate at init time. The existing
> per-variant values are moved under their calibrated rate (19.2 MHz for
> v1/v1.5, 37.5 MHz for v2), no timing values are changed.
> 
> At this stage the driver only validates the running rate against the
> table, the timings are only valid at the exact rate they were calibrated
> for, so if the current rate has no matching entry for a master's mode,
> fail initialization rather than program incorrect timings. A following
> patch actively enforces the required rate so this becomes a safety net.
> 
> Suggested-by: Vladimir Zapolskiy <[email protected]>
> Signed-off-by: Loic Poulain <[email protected]>
> ---
>   drivers/i2c/busses/i2c-qcom-cci.c | 74 +++++++++++++++++++++++++++++++--------
>   1 file changed, 59 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 0a216c4d08114f267c8441925e8811ca64f1a909..1092ce0371429ef64c7cc44cc23a42ba4c135af5 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -82,6 +82,13 @@ enum {
>   	I2C_MODE_STANDARD,
>   	I2C_MODE_FAST,
>   	I2C_MODE_FAST_PLUS,
> +	NUM_I2C_MODES,
> +};
> +
> +enum {
> +	CCI_CLK_RATE_19_2MHZ,
> +	CCI_CLK_RATE_37_5MHZ,
> +	NUM_CCI_CLK_RATES,
>   };
>   
>   enum cci_i2c_queue_t {
> @@ -117,7 +124,7 @@ struct cci_data {
>   	unsigned int num_masters;
>   	struct i2c_adapter_quirks quirks;
>   	u16 queue_size[NUM_QUEUES];
> -	struct hw_params params[3];
> +	struct hw_params params[NUM_CCI_CLK_RATES][NUM_I2C_MODES];
>   };
>   
>   struct cci {
> @@ -127,6 +134,7 @@ struct cci {
>   	const struct cci_data *data;
>   	struct clk_bulk_data *clocks;
>   	int nclocks;
> +	struct clk *cci_clk;
>   	struct cci_master master[NUM_MASTERS];
>   };
>   
> @@ -225,7 +233,34 @@ static int cci_halt(struct cci *cci, u8 master_num)
>   	return 0;
>   }
>   
> -static void cci_init(struct cci *cci)
> +static const unsigned long cci_clk_rates[NUM_CCI_CLK_RATES] = {
> +	[CCI_CLK_RATE_19_2MHZ] = 19200000,
> +	[CCI_CLK_RATE_37_5MHZ] = 37500000,
> +};
> +
> +static int cci_clk_rate_idx(unsigned long rate)
> +{
> +	int i;
> +
> +	for (i = 0; i < NUM_CCI_CLK_RATES; i++)
> +		if (cci_clk_rates[i] == rate)
> +			return i;
> +
> +	return -EINVAL;
> +}
> +
> +static const struct hw_params *cci_get_hw_params(struct cci *cci, int mode)
> +{
> +	unsigned long rate = clk_get_rate(cci->cci_clk);
> +	int ri = cci_clk_rate_idx(rate);
> +
> +	if (ri >= 0 && cci->data->params[ri][mode].thigh)
> +		return &cci->data->params[ri][mode];
> +
> +	return NULL;
> +}
> +
> +static int cci_init(struct cci *cci)
>   {
>   	u32 val = CCI_IRQ_MASK_0_I2C_M0_RD_DONE |
>   			CCI_IRQ_MASK_0_I2C_M0_Q0_REPORT |
> @@ -249,7 +284,12 @@ static void cci_init(struct cci *cci)
>   		if (!cci->master[i].cci)
>   			continue;
>   
> -		hw = &cci->data->params[mode];
> +		hw = cci_get_hw_params(cci, mode);
> +		if (!hw) {
> +			dev_err(cci->dev, "no timing for mode %d at CCI clock %lu Hz\n",
> +				mode, clk_get_rate(cci->cci_clk));
> +			return -EOPNOTSUPP;
> +		}
>   
>   		val = hw->thigh << 16 | hw->tlow;
>   		writel(val, cci->base + CCI_I2C_Mm_SCL_CTL(i));
> @@ -266,6 +306,8 @@ static void cci_init(struct cci *cci)
>   		val = hw->scl_stretch_en << 8 | hw->trdhld << 4 | hw->tsp;
>   		writel(val, cci->base + CCI_I2C_Mm_MISC_CTL(i));
>   	}
> +
> +	return 0;
>   }
>   
>   static int cci_reset(struct cci *cci)
> @@ -283,9 +325,7 @@ static int cci_reset(struct cci *cci)
>   		return -ETIMEDOUT;
>   	}
>   
> -	cci_init(cci);
> -
> -	return 0;
> +	return cci_init(cci);
>   }
>   
>   static int cci_run_queue(struct cci *cci, u8 master, u8 queue)
> @@ -488,8 +528,7 @@ static int __maybe_unused cci_resume_runtime(struct device *dev)
>   	if (ret)
>   		return ret;
>   
> -	cci_init(cci);
> -	return 0;
> +	return cci_init(cci);
>   }
>   
>   static const struct dev_pm_ops qcom_cci_pm = {
> @@ -570,6 +609,11 @@ static int cci_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, -EINVAL, "not enough clocks in DT\n");
>   	cci->nclocks = ret;
>   
> +	cci->cci_clk = devm_clk_get(dev, "cci");
> +	if (IS_ERR(cci->cci_clk))
> +		return dev_err_probe(dev, PTR_ERR(cci->cci_clk),
> +				     "failed to get CCI clock\n");
> +
>   	ret = cci_enable_clocks(cci);
>   	if (ret < 0)
>   		return ret;
> @@ -652,7 +696,7 @@ static const struct cci_data cci_v1_data = {
>   		.max_write_len = 10,
>   		.max_read_len = 12,
>   	},
> -	.params[I2C_MODE_STANDARD] = {
> +	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
>   		.thigh = 78,
>   		.tlow = 114,
>   		.tsu_sto = 28,
> @@ -664,7 +708,7 @@ static const struct cci_data cci_v1_data = {
>   		.trdhld = 6,
>   		.tsp = 1
>   	},
> -	.params[I2C_MODE_FAST] = {
> +	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
>   		.thigh = 20,
>   		.tlow = 28,
>   		.tsu_sto = 21,
> @@ -685,7 +729,7 @@ static const struct cci_data cci_v1_5_data = {
>   		.max_write_len = 10,
>   		.max_read_len = 12,
>   	},
> -	.params[I2C_MODE_STANDARD] = {
> +	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
>   		.thigh = 78,
>   		.tlow = 114,
>   		.tsu_sto = 28,
> @@ -697,7 +741,7 @@ static const struct cci_data cci_v1_5_data = {
>   		.trdhld = 6,
>   		.tsp = 1
>   	},
> -	.params[I2C_MODE_FAST] = {
> +	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
>   		.thigh = 20,
>   		.tlow = 28,
>   		.tsu_sto = 21,
> @@ -718,7 +762,7 @@ static const struct cci_data cci_v2_data = {
>   		.max_write_len = 11,
>   		.max_read_len = 12,
>   	},
> -	.params[I2C_MODE_STANDARD] = {
> +	.params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_STANDARD] = {
>   		.thigh = 201,
>   		.tlow = 174,
>   		.tsu_sto = 204,
> @@ -730,7 +774,7 @@ static const struct cci_data cci_v2_data = {
>   		.trdhld = 6,
>   		.tsp = 3
>   	},
> -	.params[I2C_MODE_FAST] = {
> +	.params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST] = {
>   		.thigh = 38,
>   		.tlow = 56,
>   		.tsu_sto = 40,
> @@ -742,7 +786,7 @@ static const struct cci_data cci_v2_data = {
>   		.trdhld = 6,
>   		.tsp = 3
>   	},
> -	.params[I2C_MODE_FAST_PLUS] = {
> +	.params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST_PLUS] = {
>   		.thigh = 16,
>   		.tlow = 22,
>   		.tsu_sto = 17,
> 

To my taste it brings the avoidable complexity in the logic, which
is then rightfully removed in the 4/5 change.

Eventually struct hw_params params should be removed from all
platform data instances, while here its diffusion becomes even
deeper, can you please consider to reorder the changes by popping
4/5 up?..

Apart of this concern, I do not see any technical flaws, to move on

Reviewed-by: Vladimir Zapolskiy <[email protected]>

-- 
Best wishes,
Vladimir