Re: [PATCH v4 4/5] i2c: qcom-cci: Share the timing table across CCI revisions

On 8/1/26 23:10, Loic Poulain wrote:
> The hw_params timing values only depend on the CCI clock rate and the
> I2C mode, not on the hardware revision: every per-variant table used
> identical values for a given [rate][mode]. Only the set of supported
> modes differs between revisions.
> 
> Move the timings into a single shared cci_hw_params[rate][mode] table
> and describe each variant's highest supported mode in cci_data with
> max_mode instead of duplicating the timing values. This removes the
> per-variant timing tables without any functional change.
> 
> Suggested-by: Vladimir Zapolskiy <[email protected]>
> Signed-off-by: Loic Poulain <[email protected]>
> ---
>   drivers/i2c/busses/i2c-qcom-cci.c | 181 +++++++++++++++-----------------------
>   1 file changed, 70 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index b6fa37a306758ec33ede6fcc39ce4412b86d9a84..21695c744502f7fb5d333f1272f3771e4d9d5508 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -124,7 +124,8 @@ struct cci_data {
>   	unsigned int num_masters;
>   	struct i2c_adapter_quirks quirks;
>   	u16 queue_size[NUM_QUEUES];
> -	struct hw_params params[NUM_CCI_CLK_RATES][NUM_I2C_MODES];
> +	/* Highest I2C mode supported by this variant. */
> +	u8 max_mode;
>   };
>   
>   struct cci {
> @@ -249,13 +250,76 @@ static int cci_clk_rate_idx(unsigned long rate)
>   	return -EINVAL;
>   }
>   
> +static const struct hw_params cci_hw_params[NUM_CCI_CLK_RATES][NUM_I2C_MODES] = {
> +	[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
> +		.thigh = 78,
> +		.tlow = 114,
> +		.tsu_sto = 28,
> +		.tsu_sta = 28,
> +		.thd_dat = 10,
> +		.thd_sta = 77,
> +		.tbuf = 118,
> +		.scl_stretch_en = 0,
> +		.trdhld = 6,
> +		.tsp = 1
> +	},
> +	[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
> +		.thigh = 20,
> +		.tlow = 28,
> +		.tsu_sto = 21,
> +		.tsu_sta = 21,
> +		.thd_dat = 13,
> +		.thd_sta = 18,
> +		.tbuf = 32,
> +		.scl_stretch_en = 0,
> +		.trdhld = 6,
> +		.tsp = 3
> +	},
> +	[CCI_CLK_RATE_37_5MHZ][I2C_MODE_STANDARD] = {
> +		.thigh = 201,
> +		.tlow = 174,
> +		.tsu_sto = 204,
> +		.tsu_sta = 231,
> +		.thd_dat = 22,
> +		.thd_sta = 162,
> +		.tbuf = 227,
> +		.scl_stretch_en = 0,
> +		.trdhld = 6,
> +		.tsp = 3
> +	},
> +	[CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST] = {
> +		.thigh = 38,
> +		.tlow = 56,
> +		.tsu_sto = 40,
> +		.tsu_sta = 40,
> +		.thd_dat = 22,
> +		.thd_sta = 35,
> +		.tbuf = 62,
> +		.scl_stretch_en = 0,
> +		.trdhld = 6,
> +		.tsp = 3
> +	},
> +	[CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST_PLUS] = {
> +		.thigh = 16,
> +		.tlow = 22,
> +		.tsu_sto = 17,
> +		.tsu_sta = 18,
> +		.thd_dat = 16,
> +		.thd_sta = 15,
> +		.tbuf = 24,
> +		.scl_stretch_en = 0,
> +		.trdhld = 3,
> +		.tsp = 3
> +	},
> +};
> +
>   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];
> +	if (ri >= 0 && mode <= cci->data->max_mode && cci_hw_params[ri][mode].thigh)

This earlier introduced sanity check for non-zero 'cci_hw_params[ri][mode].thigh'
is not needed anymore, right?

> +		return &cci_hw_params[ri][mode];
>   
>   	return NULL;
>   }
> @@ -696,30 +760,7 @@ static const struct cci_data cci_v1_data = {
>   		.max_write_len = 10,
>   		.max_read_len = 12,
>   	},
> -	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
> -		.thigh = 78,
> -		.tlow = 114,
> -		.tsu_sto = 28,
> -		.tsu_sta = 28,
> -		.thd_dat = 10,
> -		.thd_sta = 77,
> -		.tbuf = 118,
> -		.scl_stretch_en = 0,
> -		.trdhld = 6,
> -		.tsp = 1
> -	},
> -	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
> -		.thigh = 20,
> -		.tlow = 28,
> -		.tsu_sto = 21,
> -		.tsu_sta = 21,
> -		.thd_dat = 13,
> -		.thd_sta = 18,
> -		.tbuf = 32,
> -		.scl_stretch_en = 0,
> -		.trdhld = 6,
> -		.tsp = 3
> -	},
> +	.max_mode = I2C_MODE_FAST,
>   };
>   
>   static const struct cci_data cci_v1_5_data = {
> @@ -729,30 +770,7 @@ static const struct cci_data cci_v1_5_data = {
>   		.max_write_len = 10,
>   		.max_read_len = 12,
>   	},
> -	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
> -		.thigh = 78,
> -		.tlow = 114,
> -		.tsu_sto = 28,
> -		.tsu_sta = 28,
> -		.thd_dat = 10,
> -		.thd_sta = 77,
> -		.tbuf = 118,
> -		.scl_stretch_en = 0,
> -		.trdhld = 6,
> -		.tsp = 1
> -	},
> -	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
> -		.thigh = 20,
> -		.tlow = 28,
> -		.tsu_sto = 21,
> -		.tsu_sta = 21,
> -		.thd_dat = 13,
> -		.thd_sta = 18,
> -		.tbuf = 32,
> -		.scl_stretch_en = 0,
> -		.trdhld = 6,
> -		.tsp = 3
> -	},
> +	.max_mode = I2C_MODE_FAST,
>   };
>   
>   static const struct cci_data cci_v2_data = {
> @@ -762,66 +780,7 @@ static const struct cci_data cci_v2_data = {
>   		.max_write_len = 11,
>   		.max_read_len = 12,
>   	},
> -	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_STANDARD] = {
> -		.thigh = 78,
> -		.tlow = 114,
> -		.tsu_sto = 28,
> -		.tsu_sta = 28,
> -		.thd_dat = 10,
> -		.thd_sta = 77,
> -		.tbuf = 118,
> -		.scl_stretch_en = 0,
> -		.trdhld = 6,
> -		.tsp = 1
> -	},
> -	.params[CCI_CLK_RATE_19_2MHZ][I2C_MODE_FAST] = {
> -		.thigh = 20,
> -		.tlow = 28,
> -		.tsu_sto = 21,
> -		.tsu_sta = 21,
> -		.thd_dat = 13,
> -		.thd_sta = 18,
> -		.tbuf = 32,
> -		.scl_stretch_en = 0,
> -		.trdhld = 6,
> -		.tsp = 3
> -	},
> -	.params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_STANDARD] = {
> -		.thigh = 201,
> -		.tlow = 174,
> -		.tsu_sto = 204,
> -		.tsu_sta = 231,
> -		.thd_dat = 22,
> -		.thd_sta = 162,
> -		.tbuf = 227,
> -		.scl_stretch_en = 0,
> -		.trdhld = 6,
> -		.tsp = 3
> -	},
> -	.params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST] = {
> -		.thigh = 38,
> -		.tlow = 56,
> -		.tsu_sto = 40,
> -		.tsu_sta = 40,
> -		.thd_dat = 22,
> -		.thd_sta = 35,
> -		.tbuf = 62,
> -		.scl_stretch_en = 0,
> -		.trdhld = 6,
> -		.tsp = 3
> -	},
> -	.params[CCI_CLK_RATE_37_5MHZ][I2C_MODE_FAST_PLUS] = {
> -		.thigh = 16,
> -		.tlow = 22,
> -		.tsu_sto = 17,
> -		.tsu_sta = 18,
> -		.thd_dat = 16,
> -		.thd_sta = 15,
> -		.tbuf = 24,
> -		.scl_stretch_en = 0,
> -		.trdhld = 3,
> -		.tsp = 3
> -	},
> +	.max_mode = I2C_MODE_FAST_PLUS,
>   };
>   
>   static const struct of_device_id cci_dt_match[] = {
> 

-- 
Best wishes,
Vladimir